## What
Adds a "Running the server for QA" subsection to the project `CLAUDE.md`
so commands/skills that ask the user to open the app in Chrome have
clear, project-level steps.
## Steps documented
1. First run in a worktree? Prepare it with `./worktree.sh
<main-repo-path>`.
2. Start the server if it isn't running: `composer run dev`.
3. Get the dynamic per-worktree URL — `composer run dev` prints and
copies it to the clipboard on startup; don't guess it.
Docs-only change.
Removes the weekly schedule for `stats:stuck-cohort-report`. The command
stays available to run manually via artisan; it just no longer fires on
the Monday 09:00 cron.
## Problem
`getCategoryColorClasses(color)` in `resources/js/types/category.ts`
returned `colorMap[color]` directly. When a category's `color` is
**not** one of the known `CATEGORY_COLORS` (a legacy value, or any color
the frontend map doesn't cover), the lookup is `undefined`. Consumers
then read `.bg` off it — e.g. the transaction categorizer command
palette:
```tsx
const colorClasses = getCategoryColorClasses(category.color);
// ...
className={cn(..., colorClasses.bg)} // 💥 Cannot read properties of undefined (reading 'bg')
```
This threw `TypeError: Cannot read properties of undefined (reading
'bg')` and crashed the categorize-transactions view.
**Sentry:** `PHP-LARAVEL-4H` (frontend, error/high) — first seen
2026-07-14, in `use-categorize-transactions` chunk,
`getCategoryColorClasses` → `Array.map` over categories.
## Fix
Mirror the guard that already exists in the sibling
`getCategoryChartColor` (which does `?? 'var(--color-gray-500)'`): fall
back to the gray classes when the color isn't in the map.
```diff
- return colorMap[color];
+ return colorMap[color] ?? colorMap.gray;
```
One line, behavior-preserving for all valid colors, and it turns a crash
into a neutral gray swatch for the rare out-of-enum color.
## Test
Added two cases to `resources/js/types/category.test.ts`:
- a known color still returns its own classes,
- an unknown color falls back to gray instead of returning `undefined`.
Ran locally (minimal vitest config): 4/4 pass.
🤖 Found and fixed autonomously via the Sentry monitoring loop.
## Problem
On the create/edit transaction form, the amount field uses
`inputMode="decimal"`. On iOS the numeric keypad that this triggers has
**no minus key**, so users cannot type the leading `-` the input relies
on to record a negative amount (e.g. an outgoing transfer).
## Fix
Add an opt-in `allowNegative` prop to `AmountInput` that renders a small
`+`/`−` sign-toggle button at the **start** of the field. It works with
any keyboard, so it does not depend on the OS keypad. It is enabled only
on the create-transaction amount input; budgets, balances and loan
fields keep their current UI.
- The button turns red with a `−` when the amount is negative, and shows
`+` otherwise (`aria-pressed` reflects the state).
- Toggle fires on `click` (works for both pointer and keyboard);
`onPointerDown` `preventDefault` keeps the input focused to avoid a
blur/reformat race.
- A leading `-` set via the toggle **before** typing is preserved on
focus, so the sign is not silently dropped.
- The currency symbol shifts right when the toggle is present so the two
never overlap; layout is unchanged when `allowNegative` is off.
## Refactor
Extracted the repeated `evaluateMathExpression(x) ?? parseInputValue(x)`
string→cents resolution (previously duplicated across blur, Enter, and
the new toggle) into a single `resolveCents()` helper.
## Tests
`resources/js/components/ui/amount-input.test.tsx` — the toggle is
hidden unless `allowNegative`, flips positive↔negative, and preserves
the sign when focusing after toggling an empty field.
## QA
Manually verified in the running app (create-transaction dialog): typing
an amount and toggling the sign both ways updates the value and the
button state correctly, on both prefix (USD `$`) and suffix (EUR `€`)
currency layouts.
## Summary
Two improvements to the Cashflow Sankey chart, plus a fix reported for
real users whose chart rendered with overlapping labels.
### 1. Fix overlapping labels on small categories
recharts spaces same-column Sankey nodes by exactly `nodePadding` and
distributes the height **proportionally to value**, so tiny categories
end up separated by only that padding regardless of canvas height. It
was 24px while a two-line label is 30px tall, so adjacent labels
overlapped (e.g. *Regalos* / *Otros* in the monthly view, *Ingreso
desconocido* / *Pagos devueltos* in the yearly view).
Set `NODE_PADDING` above the label height so two adjacent labels can
never collide, whatever the bar size. This is independent of width, so
it fixes desktop and mobile alike.
### 2. Expand income categories (drill-down)
Income categories can now be drilled down into their subcategories,
mirroring the existing expense behaviour. The `sankey` endpoint already
returns both sides for a given `parent`, so this is frontend-only:
- The open drill-down is keyed by `{id, kind}` — a category can
legitimately appear on both the income and expense side.
- Income subcategories link `child -> parent` and form a column to the
**left** of the parent (expense subcategories stay on the right).
- The chart-height budget now accounts for the combined leftmost column
(income siblings + subcategories share it), so a many-source drill-down
can't drive recharts to a negative row scale and break the whole chart.
- The expand chevron points the way the column opens: income shows `<`
collapsed / `>` expanded; expense keeps `>` collapsed / `v` expanded.
The income/expense node-building was also de-duplicated into a single
helper.
## Test plan
- `sankey-chart.test.tsx`: 7 unit tests green (added income-expansion
coverage alongside the existing expense one).
- `tsc --noEmit`, `eslint`, `prettier` clean.
- Manual browser QA (Playwright) on a real account, desktop **and**
mobile: monthly + yearly views (overlap gone), expand income, switch to
expand expense (no regression), collapse, chevron direction, responsive
horizontal scroll.
## Demo
<!-- PLACEHOLDER: drag the demo video here -->
_📎 Attach the demo video here — file generated at
`~/Downloads/sankey-cashflow-demo.mp4`_
## Summary
A credit card is a spending account, not wealth. This change:
- **Excludes credit cards from net worth entirely** — they no longer add
to *or* subtract from the net worth total, the evolution chart, or its
trends. They are filtered out at the source, so every net-worth
aggregation (backend summary + frontend chart/trend/MoM) is consistent.
- **Shows the credit card balance as a positive figure** on the
per-account cards, on both the dashboard and the Accounts page (like any
other account).
- **Fixes a loan sign inconsistency**: a loan used to show *positive* on
the Accounts page but *negative* on the dashboard. The Accounts list now
applies the liability sign too, so loans render negative in both places.
### Why
A user reported that a credit card showed as a positive balance on the
Accounts screen but dragged their net worth down on the dashboard — the
same account rendered with opposite signs on the two screens. The root
cause was that credit cards were modelled as liabilities (like loans)
and each screen computed the balance through a different path. Product
decision: a credit card is spendable credit, so its balance is shown
as-is and simply doesn't participate in net worth.
## Behavior
| Account (e.g. 90k / 50k) | Per-account cards (dashboard + Accounts) |
Net worth chart & total |
| --- | --- | --- |
| **Credit card** | `+90,000` | **excluded** (not counted) |
| **Loan** | `−50,000` (was `+50,000` on Accounts) | subtracts
(unchanged) |
| Checking / savings / … | unchanged | unchanged (asset) |
## ⚠️ Existing-data impact
Previously credit cards were forced to **reduce** net worth
(`-abs(balance)`). With this change they are excluded, so **users with
existing credit-card accounts will see their net worth rise** by
whatever their cards used to subtract. This is a deliberate semantic
change, not a migration — no stored balances are altered.
## Implementation
- `AccountType::countsInNetWorth()` — new predicate, `false` only for
credit cards; `calculateNetWorthAt()` skips excluded types.
- `AccountType::reducesNetWorth()` / `LIABILITY_TYPES` — now only
`loan`, so `netWorthContribution()` renders credit cards positive on the
per-account cards.
- `net-worth-chart.tsx` — credit cards filtered out of
`includedAccounts` (one point that cascades to segments, totals, trends
and `useChartViews`).
- `Accounts/Index.tsx` — applies `netWorthContribution()` so loans
render negative, matching the dashboard.
## Testing
- Backend: `AccountTypeTest` (32) and `DashboardAnalyticsTest` (credit
card excluded, loan still subtracts) — green.
- Frontend: `chart-calculations.test.ts` (37) — green.
- `pint`, `prettier`, `eslint` — clean.
Browser QA skipped per request (the logged-in flow is also currently
blocked locally by pending Spaces migrations). Logic is fully covered by
the tests above.
## Problem
The standalone `pr-title.yml` workflow produced a `Conventional PR
title` status check, but that context was **not** in `main`'s
branch-protection required list (`tests`, `linter`, `static-analysis`,
`browser-tests`, `performance-tests`). Auto-merge only waits on required
checks, so a **failing** title check never blocked a merge — which is
exactly what happened.
## Fix
Fold the same validation into the `linter` job, which is already a
required check. A bad PR title now fails `linter` → blocks the merge
(and auto-merge waits). The standalone workflow is removed as redundant.
Since required checks live in branch protection (not in the repo — no
config-as-code here), hanging the validation off an already-required job
is the only way to enforce it via a PR.
## Trade-off
The title re-validates on `push`/`synchronize` rather than on the
`edited` event, so correcting a bad title after opening a PR needs a new
push (a re-run won't pick up the edited title). Acceptable for the
simpler, single-mechanism setup.
## What & why
The Cashflow "Money Flow" diagram was a bespoke SVG Sankey that laid out
nodes and links by hand. It didn't hold up on small screens (fixed 400px
min-width, cramped flows) and was ~950 lines of custom geometry to
maintain.
This replaces it with recharts' `<Sankey>` (already a dependency) inside
a `ResponsiveContainer`, letting recharts own the layout. Net **−950 /
+515** lines across the component.
## What's kept
- Income → **Net** hub → expense flow.
- Small-category **"Other"** grouping (`groupSmallCategories`).
- Privacy-mode masking on every amount (hub + categories +
subcategories).
- Click-through to a category's transactions.
- Category colors, resolved via `categoryBarColor` (same as the
breakdown cards).
## What's new
- **Drill-down (expense side):** click an expense category with children
to expand its subcategories into their own column; recharts re-lays-out
around it. One open at a time, a chevron marks expandable parents, and
the expanded parent's label moves onto its bar. Subcategories and
childless categories still click through to their transactions. Data
comes from the existing `GET /api/cashflow/sankey?...&parent=<id>`
endpoint, fetched lazily and reset when the period changes.
- **Mobile:** the chart scrolls horizontally with a comfortable
min-width (same pattern as the Trend chart) instead of crushing the
flows; canvas height grows with the busiest column so labels don't
collide.
- `align="left"` so an expanded parent's subcategories line up beside it
instead of collapsing into the last column and crossing every flow.
## What's dropped (intentional)
- The "Other" popover breakdown — the per-category detail already lives
in the breakdown cards below the chart.
- Income-side drill-down — with the central hub, expanding an income
source would split the income column across two depths (visually
broken); income categories rarely have children. Income keeps
click-through.
## Review
Two parallel reviews (technical + bugs/side-effects) ran over the diff.
Applied:
- **Category colors:** resolve the `CategoryColor` keyword via
`categoryBarColor` instead of using it raw as an SVG `fill` — non-CSS
keywords (`slate`, `neutral`, `amber`, `rose`) were rendering **black**.
(Pre-existing bug carried over from the old chart.)
- Skip top-level categories with `amount <= 0` so they don't become
stray unlinked nodes.
- Keep an expanding parent's label beside its bar until subcategories
load (no flicker); collapse again if the fetch fails.
- Cap right-side label width so a parent left of an expanded column
can't stretch its label across it.
- Memoize the cashflow `period` so the drill-down fetch effect doesn't
re-run on unrelated re-renders.
Dropped (with reason): re-adding the "Other" popover and income
drill-down (intentional, above); wrapping aria-labels in `__()` (would
add i18n keys, and matches the prior behavior); the recharts node/link
prop casts (version is pinned; params are hand-typed to document the
fields used).
## Testing
- 6 Vitest unit tests (render, "Other" grouping, empty state, drill-down
with a mocked fetch, expand affordance, privacy masking) — green.
- `tsc --noEmit` clean for the touched files; ESLint + Prettier clean.
> **Note on browser QA:** I did not record an automated demo video. The
local DB holds real user financial data (won't log in / record), and
creating a synthetic test/demo account is currently blocked by pending
Spaces migrations in the local DB (`current_space_id` column missing —
unrelated to this change). The chart was verified interactively during
development (desktop render, mobile layout, drill-down, and the
black-color fix). Happy to record a walkthrough if we seed a safe demo
account.
## Problem
A user reported the same bank transactions appearing twice.
Investigation in prod showed this is a systematic dedup bug in the
EnableBanking sync affecting a few hundred rows across several
users/accounts.
## Root cause
The affected banks don't return a real `transaction_id`; their only id
is a **positional** `entry_reference` of the form
`{booking_date}.{index}` (e.g. `YYYY-MM-DD.0`). That field is **null the
day a transaction first appears** and only **populated on a later
sync**.
`TransactionFingerprint::for()` preferred `entry_reference` when present
and fell back to a content hash when absent. So the same transaction
produced two different fingerprints:
| | First sync (same day) | Later sync |
|---|---|---|
| `entry_reference` | `null` | `YYYY-MM-DD.0` |
| `dedup_fingerprint` | content-based | id-based |
| `external_transaction_id` | `null` | `YYYY-MM-DD.0` |
Neither the fingerprint nor the `external_transaction_id` dedup path
matched across the two syncs → duplicate row. The one-shot historical
first sync has no duplicates; the duplicates start with the daily
incremental syncs.
## Fix
Treat a positional (`^\d{4}-\d{2}-\d{2}\.\d+$`) `entry_reference` as "no
stable id" and fall through to the content hash, which is identical on
both syncs. Genuine `transaction_id` and non-positional
`entry_reference` keep keying exactly as before.
## Trade-off (accepted, follow-up tracked)
The positional index is also the only field that distinguishes two
genuinely-distinct same-day transactions with byte-identical content.
Falling to the content hash collapses them to one fingerprint, so only
the first is kept — a rare silent under-count. We accept it here over
the systematic duplication it fixes; **existing rows are unaffected**
(their distinct positional value is still stored in
`external_transaction_id` and caught by the fallback dedup path on
re-sync). Fixing both cases needs occurrence-aware dedup in the consumer
(a schema change) — left as a follow-up and documented in the code.
## Tests
- Regression: a positional `entry_reference` matches the same
transaction seen earlier without one.
- Boundary: non-positional references still key on `entry_reference`.
- `php artisan test
tests/Unit/Services/Banking/TransactionFingerprintTest.php` → 6 passed.
`pint` clean.
## Data cleanup (separate, after deploy)
The already-duplicated rows still need a one-off cleanup (soft-delete
the later copy per group, keeping the content-fingerprint original). It
must run **after** this fix ships, otherwise the next daily sync
re-creates them. Not included in this PR.
## What
Reworks `stats:experiment-funnel` from a pure conversion funnel into a
**contribution-margin** one, so the 3-way trial/pricing experiment can
be judged on margin — not just conversion. Signups aren't free: each
bank connection costs money per month, so trials that connect banks and
never pay are burned cash, which the old report was blind to.
## Funnel
Per variant: **assigned → activated → carded → net-paying**
- **activated** = connected ≥1 bank connection **OR** gave AI consent
(triggered paid infrastructure), whether or not they paid.
- **carded** = completed Stripe Checkout (card on file).
- **net-paying** = live, non-refunded, mature subscription.
The **activated → carded** gap is where a user connects a bank and walks
away without paying — the exact leak the report now puts a number on.
## Metrics
| Column | Meaning |
|---|---|
| `A2P%` | net-paying ÷ activated (mature) |
| `Cost` | mature-cohort connections × cost/connection |
| `Burn` | connection cost of mature non-payers (money lost) |
| `CM` | MRR − Cost (the decision metric) |
- New `--cost-per-connection` option, default **0.40** per connection.
- Connection count includes soft-deleted/revoked connections (they still
cost money).
- All money/rate columns are gated on each variant's maturity window;
`Cost`/`Burn`/`CM` print `—` until a variant has mature volume. Legend
notes that raw `Assg`/`Actd`/`Card` are lifetime counts while
rates/money are mature-cohort only.
- Existing MRR/ARPU/Net% fields stay on the collector; dropped from the
printed table.
## Tests
Added coverage for activation (bank OR AI),
cost/burn/contribution-margin math, and the cost-per-connection
argument. Full file green (12 passed).
## Why
In the experiment funnel report, variants with a trial (control,
reduced) show `0` under Actv/Cncl/Rfnd while their signups are still
mid-trial — so the table looked empty even though many of those trials
have already been canceled by the user (Cashier keeps serving the trial
until it ends, then simply doesn't charge). That "already lost, just not
settled yet" cohort was invisible.
On prod right now: 28 subscriptions in `trialing`, of which **10 are
already scheduled to cancel** — a leading churn signal the report was
hiding.
## What
- `ExperimentFunnelCollector`: new per-variant `trialingCanceling`
counter = `stripe_status === 'trialing'` **and** `ends_at !== null`.
- Report table: two new columns — `Trl` (currently in trial, previously
collected but never printed) and `TrlX` (of those, already scheduled to
cancel and won't convert). Discord legend updated.
- Test covering the new counter.
`Trl`/`TrlX` are orthogonal leading indicators; the mature Net%/MRR/ARPU
metrics are unchanged.
## Testing
`php artisan test
tests/Feature/SendExperimentFunnelReportCommandTest.php` — 9 passed.
## Why
A balance of exactly **0** could not be saved. Zero is a perfectly valid
balance for any account (an emptied wallet, a paid-off loan, a closed
position), so the app was rejecting legitimate input.
## Root cause
The shared `AmountInput` renders a numeric value of `0` as an **empty
string** (so the field shows the `0.00` placeholder instead of a literal
"0.00"). Several balance forms combined that with the native HTML
`required` attribute — which then rejected the *only* value that renders
empty: `0`. `required` on this input never guarded against a missing
value (the state is always numeric; empty == 0), it only ever blocked
zero.
The backend already accepted `0` (`required|integer`, and Laravel's
`required` treats integer `0` as present), so this was purely a frontend
constraint.
## Changes
- Drop `required` from the balance `AmountInput` in the **Update
balance** dialog and the **Balance history** edit modal.
- Remove the explicit `balanceInCents === 0` guard (and `required`) in
the onboarding **Set balance** step.
- Extend the same fix to the CSV-import **reference balance** field
(identical root cause; the compute path already gates on
`referenceBalance !== null`, so an explicit `0` is a valid anchor).
- Remove the now-orphaned `"Please enter a balance"` translation from
`es.json` / `fr.json`.
- Add feature tests asserting a zero balance can be stored and set as
the current balance.
Transaction and budget amount inputs keep `required` — `0` is not a
meaningful value there, so they are intentionally untouched.
## QA
- **Onboarding balance step**: saving with an empty/zero field now
advances instead of showing "Please enter a balance". ✅
- **Update balance dialog**: set €1,000.00, then overwrote it with €0.00
→ dialog closed and the record persisted. Verified in the DB
(`account_balances.balance = 0`). ✅
- Backend contract locked by new tests in
`AccountBalanceControllerTest`.
## Demo
<!-- PLACEHOLDER: drag the video here -->
_Video to attach: `~/Downloads/allow-zero-balances-demo.mp4`_
## Spaces / Business plan — Phase 0: invisible foundation
First of **three stacked PRs** introducing multi-tenant **Spaces** (the
basis for the Business plan). This one is a **pure, behaviour-preserving
foundation**: it can ship to production on its own with zero
user-visible change.
- **Stacked PRs:** this → `enterprise-spaces-ui` (Phase 1+2) →
`enterprise-spaces-invitations` (Phase 3).
### What a Space is
A Space groups its own accounts, connections, transactions, categories,
labels, budgets and rules. **Every user gets one invisible "Personal"
space**, provisioned automatically on creation — so the architecture is
identical for free, Standard and Business accounts, even though only
Business will ever see more than one.
### What this PR does (no behaviour change)
- `spaces`, `space_user`, `space_invitations` tables;
`users.current_space_id`; a nullable, indexed `space_id` on the 8 owned
tables (plain column, **no FK** — avoids a validating table-scan/lock on
`transactions` during a phased rollout).
- `Space` model + `BelongsToSpace` trait that stamps `space_id` on
create (from the row's user's current space; a transaction inherits its
account's space, so bank-sync lands rows correctly).
- Idempotent, chunked `spaces:backfill` command (run from a migration)
that gives every existing user a personal space and stamps their rows —
so the read switch in the next PR is safe.
- **Reads are untouched here** (still user-scoped); every user has
exactly one space, so behaviour is identical.
### Testing
- New `tests/Feature/Spaces/SpaceFoundationTest.php` (provisioning,
default-space stamping, account-anchored transaction space,
stale-pointer self-heal, backfill).
- Full suite green.
### Notes / deliberate simplifications
- `space_id` stays **nullable** (populated by backfill + on every
write); the NOT NULL constraint is deferred until prod is confirmed
fully backfilled.
- For very large `transactions` tables, `spaces:backfill` can be run
out-of-band before deploy so the migration's call is a no-op.
## What & why
A queued historical-balance generator job exhausted the worker's 128MB
PHP memory limit and **died on every retry** — Sentry `PHP-LARAVEL-49` +
`PHP-LARAVEL-4A` (two fingerprints of the *same* poison message:
identical `trace_id`/`message.id`, `retry.count` 1 then 2, OOM at
slightly different `Arr.php` lines).
### Root cause (trace-confirmed)
The Sentry trace localized the OOMing message to
`App\Jobs\GenerateHistoricalRealEstateBalancesJob`, dispatched from
`Settings\AccountController::store` for the "older than 12 months" slice
of history. The fatal frame is `Arr::map` inside Eloquent's
`AccountBalance::upsert()` value-prep (`addUniqueIds`/`addTimestamps`
per-row `array_merge`, `Query\Builder::upsert` `Arr::flatten` bindings,
and a compiled multi-row SQL string — all **O(n)** over the rows).
`purchase_date` (`before_or_equal:today`) and `loan_start_date` (no
constraint) had **no lower bound**, so a mistyped/ancient year (e.g.
`0201`, `1080`) made the generator build a **multi-century monthly
series** and hand it all to a single `upsert`, blowing past 128MB.
## The fix (two commits)
1. **`fix(balances): upsert historical balances in batches`** — build +
upsert in batches of 500 (`UPSERT_CHUNK_SIZE`) in both
`RealEstateBalanceGeneratorService` and `LoanBalanceGeneratorService`,
instead of one giant operation. Same rows, same `(account_id,
balance_date)` conflict target — peak memory is now bounded regardless
of series length. Defense-in-depth.
2. **`fix(accounts): floor purchase/loan start dates at 1900`** — the
actual root cause: add `after_or_equal:1900-01-01` to `purchase_date`
and `loan_start_date`. Rejects the data-entry typos that drive the
runaway series (and the resulting DB bloat / misleading multi-century
net-worth ramp) while accepting any legitimately old asset in a
personal-finance app.
## Review (two agents, before this PR)
Both reviewed against the live trace and the code.
- **Root cause & fix confirmed** — the batching caps exactly the
`Arr::map`-over-N-rows frame from the trace; no independent OOM source
exists (no per-row model events; `upsert` bypasses events).
- **Chunking is correct** — the unique index `(account_id,
balance_date)` exists, so the conflict target is real; `buildDateList`
yields a strictly-increasing de-duplicated list, so batches are disjoint
(no dropped/duplicated dates, no off-by-one). The fresh per-row `id`
UUID is discarded on conflict → idempotent retries.
- **No material user-facing regression.** Balance consumption
(dashboard/net-worth) is date-range-clamped. Batching introduces a
theoretical partial-write-on-mid-job-failure window, but it's idempotent
and self-heals on retry — agents advised **against** wrapping it in a
transaction (holds locks, doesn't help memory).
Applied recommendation #2 (the date floor) as its own commit. Deferred
(both agents agree): extracting the two near-identical generators into a
shared base/trait — a larger refactor that shouldn't ride a hotfix.
## Tests
- Regression test in each service: generate a ~46-year (>500-point)
series and assert the batched writes produce no dropped/duplicated dates
and keep endpoints anchored (crosses multiple batches).
- Validation test: a pre-1900 `purchase_date` is rejected at
`accounts.store`.
Note: the PHP feature suite boots a MySQL testcontainer (Docker) that
isn't available in my environment, so these were validated by
static/`pint`/`php -l` locally and rely on CI for execution — auto-merge
is gated on green CI.
Fixes PHP-LARAVEL-49
Fixes PHP-LARAVEL-4A
## Problem
An account of type `checking`/`savings` with a genuinely **negative**
balance was:
- displayed as **positive** on the dashboard account card, and
- **added** to the net worth chart instead of subtracted,
while the **accounts page** rendered the same balance correctly as
negative.
## Root cause
The dashboard used `getAccountSign(type) * Math.abs(value)`. Liabilities
(`credit_card`/`loan`) are stored as positive magnitudes, so `-abs()`
was
correct for them — but for an **asset** holding a negative balance,
`+abs()`
flipped the sign to positive and summed it into net worth. The accounts
page
was correct because it uses the raw signed balance from
`AccountMetricsService`
with no sign transform.
## Fix
- Add `netWorthContribution(type, balance)` in `chart-calculations.ts`:
liabilities subtract their magnitude, assets keep their real sign (so an
overdrawn asset correctly reduces net worth).
- Dashboard account card (`use-dashboard-data.ts`) now shows the raw
signed
balance, matching the accounts page.
- Net worth headline total, short/long trend indicators and the
stacked-asset
total (`net-worth-chart.tsx`) use the raw signed value instead of
`Math.abs`.
## Tests
- 3 new unit tests in `chart-calculations.test.ts`, including a
regression case
for a negative asset balance. Full suite: 37 passing.
## What & why
The dashboard white-screened with React **"Maximum update depth
exceeded"** on mobile (iOS 18.7 / Opera Touch), fingerprinted in Sentry
as **PHP-LARAVEL-47** (`https://whisper.money/dashboard`).
### Root cause (upstream, recharts < 3.9.0)
The crashing first-party frame is inside recharts' internal
`CartesianChart` → `useElementOffset`
(`node_modules/recharts/es6/util/useElementOffset.js`). In < 3.9.0 that
hook:
- memoizes its measuring **callback ref** with `useCallback(...,
[lastBoundingBox.width, .height, .top, .left, ...])`, so **every
`setState` changes the ref identity → React re-invokes the ref → it
re-measures on each render**, and
- reads **viewport-relative** `getBoundingClientRect().top/left` with a
`EPS = 1px` threshold.
On mobile, any mid-render viewport shift (the iOS URL bar showing/hiding
on scroll) moves the box `> 1px` on each commit, feeding an unbounded
`render → measure → setState` loop until React aborts at the
nested-update limit and the dashboard white-screens.
This is the **same crash family** as the `ChartTooltipPortal` loop fixed
in #657, but a **distinct root cause inside recharts** (that first-party
loop is already fixed and covered by its own test; both are present on
this branch).
### The fix
Upgrade **recharts 3.7.0 → 3.9.2**. 3.9.0 rewrote `useElementOffset` to
drive measurement off a **`ResizeObserver` with a stable callback ref**
(deps no longer include the last bounding box). `ResizeObserver` fires
on element **size** changes, not on scroll/viewport-position — so the
iOS URL-bar shift no longer re-triggers measurement. This removes the
loop at its source; it does not merely mask it.
## Commits
1. `fix(chart): upgrade recharts to 3.9.2 to stop the dashboard render
loop` — the fix (bun.lock + a code comment on `ResponsiveContainer`
recording the `>= 3.9.0` requirement).
2. `fix(chart): raise recharts floor to ^3.9.0 so the crash fix can't
regress` — review follow-up: `package.json` was still `^3.7.0` (permits
the crashing versions); bumped to `^3.9.0` and regenerated
`package-lock.json` so both `bun install --frozen-lockfile` and `npm ci`
enforce the floor. Also resolves a pre-existing
bun.lock/package-lock.json divergence.
3. `test(chart): guard recharts >= 3.9.0 against a downgrade regression`
— asserts the installed recharts is `>= 3.9.0` (the honest test for a
"depend on the patched version" fix; a jsdom behavioral repro is
impossible — zero-size rects, no ResizeObserver — and would pass on the
buggy version too).
## Review (two agents, before this PR)
Both reviewed against the actual `node_modules/recharts@3.9.2` source,
not just release notes.
- **No material regression risk.** The `ChartTooltipPortal`'s two hard
dependencies still hold in 3.9.2: `.recharts-wrapper` is still emitted
and is still the default tooltip portal target, and `Tooltip` still
passes `coordinate` as `{ x, y }`. `ResponsiveContainer`'s
`initialDimension` is still supported. No API we use was
removed/renamed.
- **Types & tests:** `bun run types` produces zero recharts-attributable
errors; the chart-related JS tests (tooltip portal/position,
budget-spending-chart, stacked-bar custom-shape path) pass on 3.9.2.
### Lockfile note
The production bundle is built by `bun run build` off **bun.lock**
(pinned to 3.9.2). `package-lock.json` is consumed **only** by
`release.yml`'s `npm ci`, which runs `release-it`
(versioning/changelog/tag) and does **not** build the shipped bundle —
so its large regen diff is mechanical drift-repair and cannot affect
users. Follow-up worth considering (out of scope): consolidate on bun
and drop the vestigial `package-lock.json`.
## Why draft (not auto-merged)
The root-cause fix is high-confidence, but this is a **charting-library
bump touching all chart surfaces** and the original bug is
**mobile-only**, which I can't visually QA here. Please spot-check
before merge:
- [ ] **Mobile dashboard** (ideally iOS Safari + Opera Touch): charts
render, no white-screen, scroll is smooth, tooltips position correctly.
- [ ] **Animations**: recharts 3.9 rewrote the animation system — glance
that Area/Bar/Line still animate acceptably on data refresh (esp.
`account-balance-chart`, `budget-spending-chart`).
- [ ] **New Sentry noise**: watch for `"ResizeObserver loop completed
with undelivered notifications"` — that's a benign warning, not the loop
returning; suppress rather than treat as a regression.
Refs PHP-LARAVEL-47
## Problem
Sentry **PHP-LARAVEL-3B** — React `Maximum update depth exceeded`
crashing `/dashboard`. Regressed, 5 occurrences / 4 users (mobile
Chrome/Android and Safari), last seen today.
## Root cause
`ChartTooltipPortal` in `resources/js/components/ui/chart.tsx`
positioned the portaled tooltip in a `useLayoutEffect` with **no
dependency array**, so it ran after every render and called `setPos`. An
equality guard was the only thing preventing a `render → effect → setPos
→ render` feedback loop, and it failed once the computed position
oscillated by a sub-pixel (fractional `getBoundingClientRect` / integer
`offsetWidth`). React then aborted after 50 nested updates, taking down
the dashboard.
## Fix
1. **`651c7d72`** — Depend on the primitive `coordinate.x/.y` (and
`offset`) so the effect runs only when the cursor moves, never as a
result of its own `setPos`. `coordinate` itself is excluded on purpose:
Recharts hands a fresh object every render, so depending on the object
would reopen the loop. Extract the flip/clamp math into a pure
`computeTooltipPosition` helper (`resources/js/lib/`) rounded to whole
pixels, and unit-test it.
2. **`1c1602db`** — Component regression test: asserts the positioning
effect does **not** re-run on a re-render with an unchanged coordinate,
and **does** on a coordinate change. Verified it fails against the
pre-fix dependency-less effect.
3. **`0354a031`** — Strengthen the helper test to assert sub-pixel
positions collapse to one pixel, and correct the doc note (the
dependency array removes the loop; rounding only narrows the
oscillation).
## Review
Two independent review agents (architecture/duplication/tests +
user-facing correctness) examined the diff:
- **Correctness**: no must-fix issues. First paint is correct (measured
at `-9999`/opacity 0 before paint, capped by a viewport-relative
`max-w`, so size is accurate and there is no flash). The loop cannot
recur through the `setHidden` effect or a repeated identical coordinate.
No mobile/edge regression — the flip/clamp logic is the extracted
original.
- **Architecture**: helper placement/naming/types match conventions; no
duplicate positioning logic elsewhere to fold in; both agents' top
request (regression-test the actual fix mechanism) is addressed by
commit 2. `ChartTooltipPortal` is exported for the test, matching how
`StackedBarShape` is exported for its own test.
## Known minor residual (acceptable)
If the tooltip's **content size** changes while the cursor stays on the
exact same point *and* it sits at a viewport edge, the flip/clamp won't
recompute until the next mouse move. In practice content size changes
together with the coordinate (Recharts hover). Worst case is a one-frame
cosmetic overflow that self-corrects — strictly better than the crash it
replaces.
## Testing
`chart-tooltip-position.test.ts` (5) + `chart-tooltip-portal.test.tsx`
(2) pass. Note: the infinite loop itself can't be reproduced in jsdom
(zero-size rects), so the component test guards the mechanism (effect
does not self-retrigger) rather than the throw.
Fixes PHP-LARAVEL-3B
## What
The `encryption:delete-accounts` and `encryption:notify-removal`
commands target users who still hold legacy client-side encrypted data.
They should **never** warn or delete an account that is still being
billed.
## Changes
- Add `excludeBilledUsers()` to the shared
`FindsUsersWithLegacyEncryption` trait. It drops any user where
`hasActiveSubscriptionOrTrial()` is true — a valid subscription outside
its grace period, or an active generic trial. This reuses the existing
domain method (whose docblock already states such users must cancel
before deletion) instead of reimplementing Cashier's subscription-status
logic in SQL.
- Both commands apply the filter right after fetching their candidates.
- Eager-load `subscriptions` in the base query to avoid an N+1 when the
filter runs.
## Tests
- `it never deletes a subscribed user` (delete command)
- `it never warns a subscribed user` (notify command)
Both new tests plus the existing suite pass (8/8). Pint clean.
## Issue (Sentry PHP-LARAVEL-44 — low volume: 0 users, 1 event)
`Laravel\Ai\Exceptions\ProviderOverloadedException: AI provider [gemini]
is overloaded` (underlying Gemini HTTP 503 "high demand"), thrown from
the AI rule-suggestion generator. This is an expected, transient,
self-healing provider overload — but
`LaravelAiRuleSuggestionGenerator::generate()` called `report()` on
every failed batch, so a transient hiccup surfaced in Sentry as an
error.
## Fix
Split the per-batch catch so a `FailoverableException` (overload /
rate-limit / insufficient-credits) is logged at `warning` **without**
reporting, while every other `Throwable` is still `report()`ed. This
mirrors the existing, test-enforced handling in the sibling
`CategorizeTransactions` service.
Unchanged (deliberately preserved):
- **Partial tolerance** — suggestions from batches that succeeded are
still kept.
- **Rethrow-when-all-fail** — a genuine total failure still rethrows, so
the run is marked `Failed` (the onboarding "Try again / Skip" UI) and is
reported **once** at the run level. Non-transient batch errors are still
reported immediately.
## What I deliberately did NOT do
Two review agents converged that adding a **retry backoff** (the other
candidate fix) is not worth it here and carries real risk:
- The run **already self-heals**: failed/empty runs don't count toward
the throttle, so the user's "Try again" button re-runs immediately at
zero cost.
- A synchronous per-batch `sleep` on the single-worker `database` queue,
across up to ~10 batches, could push the run past its **120s job timeout
(failing more often, not less)** and starve other queued jobs during a
provider brownout.
So this PR is scoped to the safe, high-signal change: stop reporting an
expected transient as an error. Genuine misconfigurations (bad
model/key/quota) throw non-`FailoverableException` types and remain
fully reported.
## Testing
- New: an expected transient overload returns the successful batches'
suggestions and is **not** reported
(`Exceptions::assertNothingReported()`); an unexpected batch failure
**is** reported (`assertReported(RuntimeException::class)`).
- Full `tests/Feature/Ai` suite green (121 tests); Pint and Larastan
clean.
Fixes PHP-LARAVEL-44
---
🤖 Opened by the autonomous Sentry-triage loop. Auto-merge enabled:
low-risk, mirrors an existing tested pattern, keeps genuine failures
visible.
## Problem (Sentry PHP-LARAVEL-42)
EnableBanking's `GET /accounts/{id}/transactions` returns **HTTP 422
"Wrong transactions period requested"** when the requested date range is
wider than the bank is willing to serve. The catch-ladder in
`EnableBankingProvider::getTransactions()` only matched
401/EXPIRED_SESSION, 400/AccountNotAccessible and 400/ASPSP_ERROR, so
the 422 rethrew a **raw `RequestException`**. That:
1. escaped the per-account loop in `EnableBankingSyncer::sync` (which
only skips `InaccessibleBankAccountException`), so **every remaining
account in the connection stopped syncing too**;
2. hit the job's generic `catch (\Throwable)` → retried 3×
(deterministic, always the same 422) → connection marked **Error** and
**reported to Sentry**;
3. after the scheduled-retry budget (`consecutive_sync_failures >= 3`)
the whole connection was **dropped from scheduled sync** until a manual
reconnect.
Real user impacted (active connection). The failing request was a
~92-day window on the incremental/linked path.
## Fix (3 commits)
1. **Classify the 422** — new `WrongTransactionsPeriodException`
(`ShouldntReport`) thrown from `getTransactions()` when status is 422
and the message names the period. Also stop logging handled 422s at
`error` level in the HTTP client callback.
2. **Clamp + retry** — on that exception, `TransactionSyncService::sync`
restarts the account from page 1 with a progressively narrower window
(`90 → 30 → 7` days before `date_to`), so the user keeps as much history
as the bank will serve. `strategy='longest'` is dropped on the narrowed
retry so the explicit `date_from` is honoured; re-fetched pages are
idempotent (fingerprint dedup + date-keyed daily balances).
3. **Graceful skip** — if even the narrowest window is refused, the
syncer skips just that account (like an inaccessible account) and keeps
the connection Active, instead of failing the whole sync.
## Why draft (needs a human call, per two review agents)
The crash fix itself is well-covered and safe. What needs sign-off is
the **product tradeoff** the clamp introduces:
- **First-sync history truncation.** First sync requests
`now()->subYear()`. If the bank refuses a year, we narrow to ≤90 days
and there is **no back-fill path** (incremental syncs only move
`date_from` forward), so the skipped history is lost. This is bounded
and logged, but it is a deliberate behaviour change.
- **Incremental catch-up gap.** If the watermark is older than the
bank's servable window, the days between the watermark and the clamp are
never fetched (silent, but logged).
- **Heuristics worth a human eye:** the ladder values `[90, 30, 7]`,
dropping `strategy='longest'` on retry, and detecting the error by
`status 422 + message contains "period"` (ideally confirmed against
EnableBanking docs / a stable error code).
Low-risk per review: duplicate imports (fingerprint + `(account_id,
dedup_fingerprint)` unique index are robust to overlapping windows) and
balances (truncated, not corrupted).
## Testing
- Provider: 422 wrong-period → `WrongTransactionsPeriodException`;
unrelated 422 stays a raw `RequestException`.
- Service: clamps + retries once (asserts the clamped date and the
`strategy` drop); gives up after the ladder is exhausted; does not retry
an already-narrow window.
- Syncer: a refused account is skipped, connection stays
Active/unreported, siblings still sync.
- Full `tests/Feature/OpenBanking` + `tests/Feature/Sync` green (315
tests); Pint and Larastan clean.
Fixes PHP-LARAVEL-42
---
🤖 Opened by the autonomous Sentry-triage loop. Draft on purpose — the
data-truncation tradeoff above is a product decision for a human.
## Problem (Sentry PHP-LARAVEL-43)
`ReferenceError: Can't find variable: indexedDB` — an unhandled promise
rejection reported from production. Some browsers do not expose the
global `indexedDB` at all (Chrome on iOS in certain webviews,
private/locked-down modes). The app uses Dexie (IndexedDB) as an offline
cache for transactions, and any Dexie operation in that context throws.
Reproduced entry point: on `/register`, submitting the form runs
`transactionSyncService.clearAll()` → `db.transactions.clear()` → Dexie
references the missing global → throws. The same class of crash can fire
from every other cache touch point (`sync`, `getAll`, `getById`,
`getByAccountId`, and the cache eviction in `delete`).
IndexedDB here is only a cache — the API/Inertia is the source of truth
— so a missing store must degrade to "no local cache", never crash.
## Fix (3 commits)
1. **Guard helper** — `isIndexedDbAvailable()` (checked via `globalThis`
so referencing the global never throws) and `withDb(operation,
fallback)` in `dexie-db.ts`. `withDb` returns the fallback when
IndexedDB is missing or the op fails, and defers `db` access into a
closure so the lazy Dexie proxy never initializes on unsupported
browsers.
2. **Sync manager** — every Dexie read/write goes through `withDb`
(reads → empty, writes/`clearAll` → no-op); `sync()` early-returns a
skipped result and avoids the pointless server round-trip.
3. **Delete eviction** — only the best-effort local cache eviction in
`delete()` is guarded; the authoritative `axios.delete` stays outside
the guard so no API mutation is ever swallowed.
## Safety / complexity
- **No behavior change for browsers with IndexedDB** — `withDb` runs the
operation exactly as before (covered by tests).
- **Graceful degradation otherwise** — no crash; the app runs
online-only.
- **No swallowed mutations** — the guard wraps only Dexie calls; all
`axios` calls stay outside it.
- Reviewed by two agents: the crash does not actually block registration
(Inertia does not await `onBefore`), so this is a recurring unhandled
rejection rather than a hard block, and the fix has no user-facing
downside. Low complexity, high confidence → auto-merge.
## Testing
- New `resources/js/lib/sync-manager.test.ts` and
`resources/js/services/transaction-sync.test.ts` (7 tests): IndexedDB
missing → clearAll/sync/reads/delete-eviction no-op and never throw;
IndexedDB present → unchanged behavior.
- `bun run lint` / `bun run format` clean on the changed files. (The
repo's `tsc --noEmit` has pre-existing errors that CI intentionally does
not gate on; no new ones introduced.)
Fixes PHP-LARAVEL-43
---
🤖 Opened by the autonomous Sentry-triage loop. Auto-merge enabled:
additive guard, no behavior change for supported browsers, no data-loss
surface.
Adds a `schedule` trigger to the existing Release workflow so it runs
automatically.
- **Cron**: `0 8 * * 1` — Mondays 08:00 UTC (= 10:00 Madrid in
summer/CEST, 09:00 in winter/CET).
- **Increment**: defaults to `patch` on scheduled runs
(`workflow_dispatch` still lets you pick patch/minor/major).
- **Empty-week guard**: skips the run when there are no commits since
the last tag, avoiding empty releases.
Note: the release PR is still opened by `github-actions[bot]` with
`GITHUB_TOKEN`, which does not trigger required checks — merging it
stays a manual step unless a PAT/App token is wired in.
## Problem
The delete button on a saved filter was only revealed on **hover**. On
desktop that's fine, but on touch devices the button is invisible yet
still clickable, so users kept deleting saved filters by accident. There
was also no confirmation step before a filter was deleted.
## Changes
- **Always visible on touch** — the hover-reveal is now gated behind
`@media (hover: hover)`. On hover-capable devices the trash icon still
appears only on hover; on touch devices (`hover: none`) it stays
visible.
- **Confirm before deleting** — deleting a saved filter now opens an
`AlertDialog` confirmation ("Are you sure you want to delete "…"?"),
matching the existing delete-confirmation pattern used across the app.
Cancel/Escape dismiss without deleting.
## QA
Tested end-to-end in a real browser (mobile emulation, `hover: none`):
- Desktop (`hover: hover`): delete button computes `opacity: 0` until
hover ✓
- Touch (`hover: none`): delete button computes `opacity: 1`, visible
without hover ✓
- Tapping the trash icon opens the confirmation dialog with the correct
filter name ✓
- **Cancel** keeps the filter (verified in DB) ✓
- **Delete** removes it (verified in DB) ✓
## Demo
<img width="1265" height="1277" alt="qa-01-desktop-dropdown"
src="https://github.com/user-attachments/assets/29915c68-0d6e-49da-839d-ee75462338ae"
/>
<img width="1265" height="1277" alt="qa-02-confirm-dialog"
src="https://github.com/user-attachments/assets/a1bee93c-e4fe-4652-a545-35136b427772"
/>
<img width="1265" height="1277" alt="qa-03-after-delete"
src="https://github.com/user-attachments/assets/c7d66a4c-d27a-4d04-8506-05d797d4630f"
/>
<img width="389" height="844" alt="qa-04-mobile-dropdown"
src="https://github.com/user-attachments/assets/9d172e69-3a34-4fdf-a6ce-5771be23f45b"
/>
## What & why
Fixes **PHP-LARAVEL-41** — `TypeError: addEventListener is not a
function` on Safari 13.1.2 / macOS 10.13.
### Root cause
`MediaQueryList.addEventListener` does not exist on Safari <14 (and
other legacy browsers) — the property is `undefined`; those engines only
expose the deprecated `addListener`. `initializeTheme()` runs at app
boot (`resources/js/app.tsx`) and called:
```ts
mediaQuery()?.addEventListener('change', handleSystemThemeChange);
```
The `?.` guarded a **null** query but not the **missing method**, so the
call threw during module evaluation, aborting the rest of boot
(`initializeChartColorScheme`, `createInertiaApp`) → the whole React app
failed to mount (white screen) for those users. `use-mobile.tsx` had the
same latent crash for any component using `useIsMobile`.
## Changes (one concern per commit)
1. **fix(appearance): support MediaQueryList change events on legacy
Safari** — add `resources/js/lib/media-query.ts`
(`addMediaQueryListener` / `removeMediaQueryListener`) that fall back to
`addListener` / `removeListener` when the modern API is absent, and
route the `prefers-color-scheme` (`use-appearance`) and
mobile-breakpoint (`use-mobile`) subscriptions through them. Colocated
Vitest test covers both the modern and legacy-fallback branches for add
and remove.
2. **harden: no-op when neither API is present** — guard the deprecated
fallback with a `typeof … === 'function'` check so a hypothetical
`MediaQueryList` exposing neither API silently no-ops instead of
re-introducing the crash. (Reviewer recommendation.)
Modern-browser behavior is byte-for-byte unchanged (the modern path is
taken whenever `addEventListener` exists); the only added cost is a
`typeof` check.
## Verification
- `vendor/bin/pint`-equivalent for JS: Prettier + ESLint clean on all
changed files.
- Vitest: `resources/js/lib/media-query.test.ts` covers modern + legacy
+ neither-API branches. (CI runs `bun run test`.)
- Reviewed by two independent agents (architecture + product): both
concluded **ship it**, no must-fix. Confirmed no other
`matchMedia(...).addEventListener('change')` call sites were missed
(`account-balance-card.tsx` and `welcome.tsx` only read `.matches`),
correct add/remove pairing preserved, and no modern-browser regression.
## Follow-up (out of scope, flagged for awareness)
- The Vite build sets no explicit `build.target`/browserslist, so the
default baseline is Safari 14. This PR stops the reported **boot**
crash, but lazily-loaded page chunks could still `SyntaxError` on
navigation for Safari 13 users. Full Safari-13 support would need an
explicit lower build target — a broader change with its own risk, left
as a separate follow-up.
## What & why
Fixes **PHP-LARAVEL-2D** — `MaxAttemptsExceededException:
CategorizeUncategorizedTransactionsJob has been attempted too many
times.` (recurring, still firing as of yesterday, ~18 events).
### Root cause
The `database` queue connection used `retry_after = 90s`, but several
jobs declare a `$timeout` far above it (up to **600s**). `retry_after`
is **per-connection**. When a job outlives its 90s reservation, the DB
queue driver hands it to a second worker. For a `tries = 1` job that
immediately raises `MaxAttemptsExceededException` — and, worse, the body
can execute twice. `CategorizeUncategorizedTransactionsJob` runs on the
`ai` queue, which the deploy runs with **two concurrent workers**
(`docker/supervisor/supervisord.conf`), so the double-run re-bills
Gemini and resets progress — the exact harm its `tries = 1` was meant to
prevent.
At `retry_after = 90` **8 jobs** violated the invariant (600s rule jobs,
300s AI jobs, 120s sync jobs).
## Changes (one concern per commit)
1. **fix(queue): raise `retry_after` above the longest job timeout** —
default `90 → 900s` (env-overridable via `DB_QUEUE_RETRY_AFTER`),
clearing the 600s longest job with margin. Adds `QueueConfigTest`
asserting every `app/Jobs` `$timeout` stays `< retry_after` so a new
long job can't silently re-break it.
2. **fix(ai): de-duplicate the backfill per user** — `retry_after` stops
one dispatch being re-reserved, but not a *duplicate* dispatch (double
"Enable AI" click / re-enable mid-run) on the 2-worker `ai` queue.
Implement `ShouldBeUnique` keyed on user id, mirroring the sibling
`RetryTransientAiCategorizationJob`.
3. **test(queue): widen the guard** to also scan queued Mailables
(`app/Mail`) and Notifications (`app/Notifications`), which share the
connection and honor `$timeout`.
4. **docs(queue): correct the `retry_after` comment** — per-job
`$timeout` takes precedence over the worker `--timeout` (which is
currently unset), so the earlier "must sit between" phrasing was
inaccurate.
## Verification
- `QueueConfigTest` fails at the old `retry_after=90` (8 offenders) and
passes at `900`.
- `vendor/bin/pint --test` clean; 131 tests across `tests/Feature/Ai`,
AI-consent flow, and queue config pass.
- Confirmed against the real deploy: 2 workers on the `ai` queue,
`pcntl` installed (SIGALRM enforced), workers set no `--timeout` so each
job's own `$timeout` bounds execution below 900s.
## Reviewed by two independent agents (architecture + product)
Both concluded **ship it**. The double-dispatch gap they flagged is
closed by change #2. Neither found a correctness regression.
## Operational notes (no code change needed, flagged for awareness)
- **Reclaim window:** `retry_after` is per-connection, so
`emails`/`default` also inherit the 90→900s window. A job orphaned by a
*hard* worker crash (OOM/SIGKILL) is now reclaimed after up to ~15 min
instead of ~90s. Graceful deploys (SIGTERM + `stopwaitsecs=3600`) don't
orphan jobs, so this only bites on hard crashes — an acceptable trade. A
dedicated connection for the long AI queue would remove it; left as a
possible follow-up.
- **Worker `--timeout`:** the fix relies on `pcntl` staying installed
and long jobs keeping a `$timeout`. Setting an explicit `--timeout` (<
900) on the supervisord `queue:work` commands would make it
self-enforcing; not done here to avoid changing short-job hang
detection.
- If prod sets `DB_QUEUE_RETRY_AFTER` explicitly, ensure it's above the
longest job timeout (≥ 600, ideally 900).
## What
Add the Ghanaian Cedi as a supported currency.
## Note on GHC vs GHS
This was requested as "GHC". `GHC` is the **deprecated** pre-2007 code:
the conversion provider (`@fawazahmed0/currency-api`) still exposes it,
but its rate is scaled **×10 000** versus the modern cedi (≈113,749
GHC/USD vs ≈11.37 GHS/USD), because Ghana redenominated in 2007 (1 GHS =
10 000 GHC). Adding GHC would leave real-world balances inflated 10
000×. The current ISO 4217 code is **GHS**, which the provider covers at
the correct rate — so we add GHS.
## Compatibility
Provider covers `ghs` (standard ISO 4217). The conversion service
lowercases codes and fetches `ghs.min.json` — same path RSD/NZD use.
`exchange_rates` stores rates as JSON, so any 3-letter code works.
Validation rules and Inertia currency props auto-derive from config.
## Changes
- `config/currencies.php` — GHS entry (`allows_primary` +
`allows_account`)
- `lang/es.json` — Spanish translation
- `lang/fr.json` — French translation
Follows the RSD addition (#567) exactly; no code changes needed.
## Summary
Wave 2 structural refactor: the same financial math was copy-pasted
across the
dashboard and the analytics API endpoints, so it could silently diverge
between
screens — a real risk in a finance app. This consolidates the duplicated
calculations into single sources of truth, kills a net-worth N+1, and
aligns
the PHP/TS rule engines and the `TransactionSource` enum. Every change
is
**behavior-preserving**; the numeric outputs of every endpoint are
unchanged and
are locked down with characterization/parity tests.
Builds on merged #640 (Wave 1); no file overlap. No dependencies
changed.
## Changes (per commit)
- **Consolidate savings-rate math into `CashflowSummaryService`** —
`savings_rate`
and `net` were byte-identical inline in `DashboardController` and
`Api/CashflowAnalyticsController`. Extracted to
`CashflowSummaryService::summarize(income, expense)`;
both controllers now spread its result (same keys, same values).
- **Move income/expense-side classification onto the `Transaction`
model** —
the income/expense side test was reimplemented in three places
(`Api/TransactionAnalysisController`, `Api/CashflowAnalyticsController`,
`DashboardController`). Now `Transaction::isIncomeSide()` /
`isExpenseSide()`.
- **Extract duplicated `getCategorySpending` into
`CategorySpendingService`** —
the tree-rollup expense-spending query was duplicated verbatim between
`DashboardController` and `Api/DashboardAnalyticsController`. Moved to
`CategorySpendingService::forPeriod()` (drill-parent parameterized).
- **Batch net-worth balance lookups to kill the per-account N+1** —
`Api/DashboardAnalyticsController::calculateNetWorthAt` ran one
`AccountBalance` query per account per compared period. Now uses the
existing
`BalanceLookup::forAccounts()` (fixed 3 queries via carry-forward seed +
in-range records), reproducing the exact "latest balance <= date, else
0"
semantics.
- **Align server rule normalization with the client and lock it with
parity
fixtures** — `AutomationRuleService::normalizeRuleJson` protected only
`['description','notes']` while `rule-engine.ts` also protected
`creditor_name`/`debtor_name`. Aligned to the superset (structurally a
no-op
since those var names are already lowercase, so no matching change) and
added
shared PHP+TS parity fixtures so the two engines can never drift
unnoticed.
- **Add missing `TransactionSource` cases to the TS type** —
`transaction.ts`
was missing `enablebanking`/`wise`; now mirrors
`App\Enums\TransactionSource`.
- **Guard `sumTransactions` against unsupported category types**
(reviewer fix) —
replaced the income/expense ternary that silently treated any non-Income
type
as expense with a `match` that throws on Savings/Investment/Transfer.
- **Document category eager-load expectation on `Transaction` side
methods**
(reviewer fix) — doc-only note to prevent a future N+1.
## Test plan
New tests:
- `tests/Unit/Services/CashflowSummaryServiceTest.php` —
net/savings-rate/rounding/div-by-zero.
- `tests/Feature/TransactionSideClassificationTest.php` — income/expense
side across signs, uncategorized, and transfer/savings/investment =
neither side.
- `tests/Feature/DashboardAnalyticsTest.php` — new net-worth test
asserts both the values (600000 / 540000) and a flat balance-query count
(<= 3) regardless of account count.
- `tests/Feature/RuleEngineParityTest.php` +
`resources/js/lib/rule-engine-parity.test.ts` +
`tests/Fixtures/rule-engine-parity.json` — one shared fixture set
driving both the PHP and TS rule engines.
Results (targeted, local):
- `--filter=Cashflow` (exclude Browser): 57/57 passed
- `--filter=DashboardAnalytics`: 41/41 passed
- `--filter=AutomationRule`: 60/60 passed
-
`--filter=TransactionSideClassification|CashflowSummaryService|RuleEngineParity`:
21/21 passed
- `bun run test rule-engine` (vitest): 13/13 passed
- `vendor/bin/pint --test`: pass; `bun run lint`: 0 errors; `bun run
format:check`: clean
- `bun run types`: 157 errors (unchanged pre-existing baseline), 0 in
touched files
Note: the 6 `Cashflow*` Browser tests fail locally only on "Vite
manifest not found" (no build present); they are environmental, not
logic, and pass in CI.
## Reviewer findings
Two read-only reviewers (architecture/quality and product/behavior)
reviewed the diff.
**Addressed**
- Both flagged that `sumTransactions` silently treated any non-Income
type as expense — added a throwing `match` guard.
- Eager-load expectation documented on the `Transaction` side methods.
**Verified identical** (behavior reviewer):
income/expense/net/savings_rate across all three endpoints; net worth
for both compared dates including no-record / all-records-after-range /
same-date edge cases; category spending (uncategorized excluded,
soft-deleted categories excluded, rollup/drill preserved); rule-engine
normalization output; multi-currency conversion.
**Deferred (documented)**
- The income/expense **summation** itself is still computed three ways
with differing uncategorized-transaction handling (dashboard
`whereExists` + sign vs analytics `join` excluding uncategorized vs
in-memory `isIncomeSide`). Unifying it would change numbers, so it is
out of scope for this behavior-preserving PR — worth a dedicated
follow-up.
- `savings_rate` keeps its `int|float` union (int `0` when income is 0).
Intentionally preserved to keep JSON output byte-identical.
- `BalanceLookup`'s `empty()` guard never short-circuits a `Collection`,
so a zero-account user runs 3 empty (harmless) queries. Left untouched —
it lives in a shared, unchanged service and only affects a no-account
edge case.
Do not merge before Wave 1 (#640) is in main.
## Summary
Adds the Nigerian Naira (NGN, ₦) as a selectable currency.
Conversion support required no code change: rates come from the
fawazahmed0 currency-api CDN, which already serves NGN (verified live,
e.g. `EUR→NGN ≈ 1566.8`). `ExchangeRateService` /
`CurrencyConversionService` are currency-agnostic, so NGN converts as
soon as it's a valid option.
## Changes
- `config/currencies.php` — add NGN entry (allowed as both primary and
account currency).
- `resources/js/utils/currency.ts` — add `₦` to the symbol map.
- `lang/es.json` — Spanish translation for the currency name.
- `tests/Feature/CurrencyOptionsTest.php` — assert NGN is exposed as a
primary and account currency.
## Testing
- `php artisan test tests/Feature/CurrencyOptionsTest.php
tests/Feature/CurrencyConversionServiceTest.php
tests/Feature/LocalizationTest.php` — pass
- `vendor/bin/pint`, `bun run format`, `bun run lint`, `vitest run
currency.test.ts` — pass
## Summary
Wave 1 hardening: privacy/security quick wins on the Inertia boundary, a
CI safety net, and stricter test isolation. Four focused changes plus
two review fixes, each in its own commit. No dependency or
product-behavior changes.
## Changes (by commit)
1. **Hide sensitive User fields from serialization** — `User::$hidden`
only covered password / 2FA / remember_token, leaving Cashier billing
columns (`stripe_id`, `pm_type`, `pm_last_four`, `trial_ends_at`) and
the legacy `encryption_salt` exposed in every serialized User, including
the Inertia-shared `auth.user` prop. None are read by the frontend and
Cashier keeps reading them server-side, so hiding them is invisible to
the UI and billing.
2. **Advisory frontend type-check in CI + duplicate `currency_code`
fix** — the CI linter job never ran `tsc`, so type errors accumulated
unseen. Adds a `Type Check Frontend` step running `bun run types`. It is
`continue-on-error: true` on purpose: the codebase already carries ~157
pre-existing `tsc --noEmit` errors, so gating on it now would turn CI
red on unrelated code. It surfaces type output today and should be
flipped to blocking once the backlog is cleared. Also removes a
duplicate `currency_code` member on the `User` TS interface (declared
twice, `CurrencyCode` and `string | null`); the TS language server flags
it, `tsc` masks it under `skipLibCheck`.
3. **Move residual-encryption cleanup out of Inertia `share()` into a
queued job** — `share()` is a shared-data provider and must be
read-only, but it ran a `DELETE`+`UPDATE` against the user on every
non-API web GET to purge the leftover encryption salt /
`EncryptedMessage` once a user had no encrypted data left. The existing
`encryption:*` commands do not cover this case (both target users who
*still* have encrypted data; this finalizes users who *finished*
decrypting). The work now goes to a new idempotent
`PurgeResidualEncryptionArtifactsJob` dispatched from `share()`,
preserving the eventual-cleanup semantics without writing during the
render.
4. **Block stray HTTP in the Feature test suite** — adds
`Http::preventStrayRequests()` in a Feature-scoped `beforeEach` so any
unfaked outbound request fails loudly instead of hitting the network. A
representative HTTP-touching subset (open banking,
exchange-rate/currency, AI categorization + AI/stats reports, analytics,
Discord, Stripe, bank logos) was run with the guard active; no test
relied on a real request, so no fakes had to be added.
### Review fixes (after the two-reviewer pass)
5. **Purge check uses `name_iv` source of truth, not the stale
`encrypted` flag** — the destructive purge decided "no encrypted
accounts" from `accounts.encrypted`, a flag the codebase already treats
as unreliable (see the
`align_accounts_encrypted_flag_with_plaintext_names` migration and
`FindsUsersWithLegacyEncryption`, which key off `name_iv`). An account
with an encrypted name but a stale `encrypted=false` flag could have its
key material destroyed. The job's guard now matches the canonical `*_iv`
predicate exactly; the loose flag gate in `share()` is kept only as a
cheap dispatch filter, and the job re-verifies with the safe predicate
before touching anything.
6. **Deduplicate purge dispatches with `ShouldBeUnique`** — `share()`
runs on every web GET, so an affected user re-enqueued the job on each
page load until a worker cleared the salt. The job is now
`ShouldBeUnique` keyed by user id, collapsing repeat dispatches into one
pending job.
## Test plan
- New/updated tests, all green:
- `InertiaSharedDataTest`: `auth.user` omits all sensitive fields; a web
GET no longer mutates the user inline and instead queues the cleanup
(and does not queue it when there is no salt).
- `PurgeResidualEncryptionArtifactsJobTest`: clears salt + message when
no `*_iv` data remains; keeps them when an encrypted transaction, an
encrypted account name, or a stale-flag-but-encrypted-name account
exists; no-op when salt already null.
- `StrayHttpRequestGuardTest`: an unfaked request throws
`StrayRequestException`; a matched fake still resolves.
- Green locally: `vendor/bin/pint --test`, `bun run lint` (0 errors),
`bun run format:check`, `bun run test` (254 frontend tests), targeted
backend
`--filter=InertiaSharedData|PurgeResidualEncryptionArtifactsJob|StrayHttpRequestGuard|Encryption`
(24 tests). Ran ~700 HTTP-touching Feature tests with the stray-request
guard active with no guard-induced failures.
- `bun run types` still reports the ~157 pre-existing errors (unchanged
set; this PR adds none) — that is exactly why the CI step is advisory
for now.
## Reviewer findings — addressed vs deferred
**Addressed**
- 🟠 Destructive purge keyed off the stale `accounts.encrypted` flag
instead of the `name_iv` source of truth → fixed in commit 5 (job now
mirrors `FindsUsersWithLegacyEncryption`; added a regression test for
the stale-flag case).
- 🟡 Per-request dispatch amplification with no dedup → fixed in commit 6
via `ShouldBeUnique`.
**Deferred (with rationale)**
- 🟠 "Three divergent copies of the legacy-encryption query" —
substantively resolved: the job now matches
`FindsUsersWithLegacyEncryption` exactly. The only remaining
`encrypted`-flag use is the `hasEncryptedAccounts` **UI prop** in
`share()`, which is a separate, pre-existing frontend concern; changing
it would alter which accounts the UI treats as encrypted and is out of
scope here. A full extraction into one shared scope would require
restructuring the trait (it builds a `User` query, not a per-model
boolean) and is not a Wave 1 quick win.
- 🟢 Redundant `->fresh()` / null guard in the job — kept deliberately:
it is the idempotency guard that makes the re-check read committed state
on the sync path (the second reviewer credited it as what makes repeat
dispatches safe).
- 🟢 `continue-on-error` shows the type-check step green — acknowledged;
flipping to blocking (or failing on an increase over a committed
baseline) is the follow-up once the ~157-error backlog is cleared.
- 🟢 (Product review) Theoretical one-request SSR/client
`hasEncryptionSetup` diff from async salt clearing — invisible in
practice (the lock button gates on `hasEncryptedAccounts ||
hasEncryptedTransactions`, false in both SSR and client), and `ssr.tsx`
is untouched. No action.
Product-bug reviewer verdict: no user-facing regressions. Hiding the 5
fields does not affect Cashier (raw attribute access),
`EncryptionController`, notifications, or any API/JSON path; the
`currency_code` dedup is runtime-identical; the cleanup timing change is
client-absorbed.
## Why
The drip email family was near copy-paste. Each of the eight mailables
repeated the same constructor, `$tries`/`$backoff`, `drip_from`
envelope, `markdown` + `userName` content, and `RateLimited` middleware
— differing only in subject and template (plus a reply-to on one, an
extra view var on another). Each of the eight jobs repeated the
`canReceiveEmails` / `hasReceivedEmail` guards, the `Mail::send` call,
and the `UserMailLog::create` block — differing only in the email type,
the mailable, and a handful of per-email eligibility checks.
## What
Two abstract bases, so each subclass declares only what varies:
- **`DripMail`** — owns the shared mailable shape. Subclasses declare
`dripSubject()` and `template()`; optional `contentData()` (PromoCode's
`FOUNDER` var) and `repliesToSender()` (PaywallFollowUp) hooks. The
hooks are named `dripSubject()`/`template()` deliberately to avoid
colliding with `Illuminate\Mail\Mailable`'s existing public
`subject()`/`markdown()` methods.
- **`SendDripEmailJob`** — owns the shared `handle()` flow (shared
guards → send → `UserMailLog`). Subclasses declare `emailType()` and
`buildMail()`; an optional `shouldSend()` hook carries the per-email
eligibility rules (pro-plan, onboarding, transactions, bank connections,
AI consent).
Each mailable drops from ~68 to ~13 lines and each job from ~48 to ~18.
Net **−459 lines**, with the send/log logic and envelope construction
now in one place.
## Behavior
No functional change. Subjects, `from`/`reply-to`, queue name, rate
limiting, templates, view data, and every per-job eligibility guard are
preserved. Notably, passing `replyTo: []` for the seven non-reply mails
is identical to the previous omission — `Envelope`'s `replyTo` defaults
to `[]`. Verified: `pest --exclude-testsuite=Browser` **1872 passing, 0
failing**; `pint --test`, `phpstan` (level 5), `format`, `lint` clean.
## Review-driven follow-up commits
Two review agents (architecture/duplication/coverage, and
behavior/regressions) ran against the first commit. The behavior agent
verified all eight jobs' guard logic invert correctly (order +
negations) and that `replyTo: []` ≡ omitted — no regressions. The
coverage agent found the two hook-driven behaviors were untested;
applied, each as its own commit:
- Extended the drip-sender parity test to all eight mailables and
asserted `PaywallFollowUpEmail`'s reply-to (and that a default drip mail
sets none).
- Asserted `PromoCodeEmail` injects the `FOUNDER` promo code into its
view data.
## Deliberately out of scope
- Non-drip mailables (`Waitlist*`, `Banking*`, `UpdateEmail`, …) share
the same `RateLimited('emails')->releaseAfter(1)` middleware and could
later share a queued-mail base too — separate concern.
- **Pre-existing** (not introduced here, noted by review): the job sends
the mail before writing `UserMailLog`, so a failure between the two can
re-enqueue a duplicate; there is no lock/unique constraint on `(user_id,
email_type)`. Worth a separate hardening pass.
## Why
The five API-key OpenBanking "connect" controllers — Binance, Bitpanda,
Coinbase, Indexa Capital and Interactive Brokers — each carried a
~60-line `store()` that was near-identical: subscription gate →
credential validation → `Bank::firstOrCreate` → create the
`BankingConnection` (Pending) → update it to `AwaitingMapping` with
pending accounts → auto-map during onboarding or redirect to mapping.
Any change to that flow, or a bug in it, had to be made five times.
## What
Extract the shared flow into a small class hierarchy, so each controller
declares only what actually varies per provider.
- **`OpenBankingConnectController`** (abstract) owns the whole flow in a
`connect()` template method, with per-provider extension points:
`provider()`, `providerName()`, `bankLogo()`, `aspspCountry()`,
`fetchProviderData()`, `credentialErrorMessage()`,
`buildPendingAccounts()`, and an optional `emptyProviderDataMessage()`
guard.
- **`CryptoPortfolioConnectController`** (abstract, extends the above)
implements the two hooks the crypto providers share: `aspspCountry()`
(from the request) and the single "Crypto Portfolio"
`buildPendingAccounts()` (uid derived from the provider enum value, so
the generated uids are unchanged).
- The five controllers shrink to their genuine differences (client,
names, logo, error copy, and — for IB — the Flex error mapping and
empty-statement guard).
Net **−110 lines** across the five `store()` methods, and the connection
lifecycle now lives in one place.
## Behavior
No functional change. The credential-failure warning log is now a single
structured message with a `provider` key instead of five free-text
variants (the only observable difference; HTTP responses are
byte-for-byte identical). Verified by the full suite: **1869 passing, 0
failing** (`--exclude-testsuite=Browser`); `pint --test`, `format` and
`lint` clean.
## Review-driven follow-up commits
Two review agents (architecture/duplication/coverage, and
behavior/regressions) ran against the first commit. Applied, each as its
own commit:
- Finished the crypto dedup via the intermediate base (the reviewers
flagged the still-duplicated crypto hooks).
- Dropped the over-engineered per-provider
`validationFailureLogMessage()` hook in favor of a structured base log.
- Added the missing test for the Interactive Brokers empty-statement
guard (the only conditional hook, previously unexercised): asserts 422 +
NAV-section message, no connection row, and no warning logged.
## Deliberately out of scope
- **Wise** is left as-is: it builds pending accounts mid-flow (needs the
client), creates the connection in one step, and has its own
empty-portfolio guard — it does not fit this template.
- **Pre-existing** (not introduced here, noted by review):
`Bank::firstOrCreate` returns an existing bank without refreshing its
logo, so `aspsp_logo` can copy a stale/null value. Worth a separate fix.
Adds the `laravel/ai` (v0) package and the `ai-sdk-development` skill to
the Laravel Boost guidelines section of CLAUDE.md, reflecting the
current installed ecosystem.
## Problem
The Discord **Plan changed** notification only showed the *new* plan
value in the `Changed` field, so it was impossible to tell what actually
changed:
```
Changed
Plan: €3.99 / month
```
Was it a price change? An interval change? From which plan? No way to
know.
## Fix
Compute the previous plan from Stripe's `previous_attributes` and render
it as `old → new`, matching the existing status-change behaviour:
```
Changed
Plan: €9.99 / month → €3.99 / month
```
`planLabel()` was generalised to read both the modern
`items.data[].price` shape and the legacy top-level `plan` shape Stripe
still includes in the diff, so the same helper works on both the current
object and the `previous_attributes` diff. Falls back to the old `Plan:
X` output when no previous value is detectable.
## Test
Added a feature test asserting the `old → new` output for a
`customer.subscription.updated` plan change. All 11 tests in the file
pass.
## Summary
- Add a new landing-page testimonial from Francisco Montes, sourced from
user feedback received by email.
- Add the matching Spanish translation in `lang/es.json` (required by
the translation CI check).
The quote was edited for clarity while keeping the user's own words and
intent. It sits just before the co-owner note, which stays last.
## Testing
- `python3 -c "import json; json.load(open('lang/es.json'))"` (valid
JSON)
- Full test suite not run locally: this worktree has no `vendor/`
installed. CI will run `./vendor/bin/pest` including the translation-key
check.
## Why
Two issues surfaced from a real mis-categorization corrected via the
**Edit Transaction** modal (not the table quick-edit).
### 1. The modal never told the user the AI learned
Both the table quick-edit and the edit modal hit the same backend
(`PATCH /transactions/{id}` → `CategoryOverrideHandler` →
`AiRuleLearner`), so both learn a forward rule from a correction. But
the modal **discarded** the update response and never read
`learned_rule`:
- No *"Learned: similar transactions will be categorized automatically"*
confirmation.
- No **Undo**.
- It still showed the generic *"Automatize"* prompt — offering to create
a rule that already exists.
The modal now reads `learned_rule` and mirrors the table's toast + undo,
skipping the automatize prompt when a rule was learned. Same i18n
strings as the table (no new keys).
### 2. The learner could key a rule on a single weak token
A correction on `"Pago en MADRID SUC 04 MADRID ES"` produced a rule
keyed on `suc` alone. The tokenizer drops noise by **per-user** document
frequency, so `madrid` (20% of this user's descriptions) is correctly
dropped — but `suc` (0.14%) survives, even though it is a generic
banking abbreviation that matches broadly as a substring. The per-user
filter can't see that `suc` is generic in general.
`AiRuleLearner::descriptionClause()` now refuses a single distinctive
token shorter than 5 characters. It learns only when there are **two or
more** distinctive tokens, or a single token of **at least 5
characters**. Merchant-keyed rules are unaffected.
## Tests
- `does not learn a description rule from a single short token`
- `learns a description rule from a single sufficiently long token`
- `learns a description rule from two short tokens`
All `AiRuleLearnerTest` pass (13/13).
## Not in this PR
The one already-learned prod rule still carries the stale `suc`
clause/title (low impact, matches ~2 txns). Cleaning that data is a
separate manual step.
## What
Two artisan commands to retire the remaining legacy client-side
encrypted data in production (transactions with
`description_iv`/`notes_iv`, accounts with `name_iv`).
- **`encryption:notify-removal`** — queues a re-engagement/deletion
warning to every user who still has an encrypted transaction or account.
Reuses `SendUpdateEmailJob` (queue `emails`, dedup via `UserMailLog`,
50/day pacing). The email is framed as an **inactivity** cleanup (no
mention of encryption) and pitches recent additions (AI insights, bank
integrations, etc.).
- **`encryption:delete-accounts`** — soft-deletes and anonymizes
(`User::markAsDeleted()`) those users **only** if they have not signed
in within the grace window (`--days`, default 7, via `last_active_at`).
Anyone who came back is spared, and the demo account is always excluded.
Both support `--dry-run` and `--force`, and share the target query via
the `FindsUsersWithLegacyEncryption` concern so the warning and the
deletion always target the same set.
## Intended manual run
`notify-removal --dry-run` → run for real → wait 7 days →
`delete-accounts --dry-run` → run for real.
## Notes / scope
- Deletion is a **soft-delete + email anonymization** (accounts become
unusable); it does not hard-purge rows, and does not cancel Stripe
subscriptions or revoke bank connections (unlike `user:delete`). Legacy
inactive accounts are unlikely to have either; handle separately if
needed.
- Email copy is plain English (matches the existing `mail/updates/*`
convention), no new `__()` keys.
## Tests
`tests/Feature/Console/NotifyEncryptedDataRemovalCommandTest.php` and
`DeleteEncryptedDataAccountsCommandTest.php` — 6 passing (targeting,
queue, dry-run, grace window, demo exclusion).
## Problem
Since #632 the account show page loads its transaction ledger via a
**deferred** Inertia prop. On first load of a transactional account the
user saw **two different loading skeletons back-to-back**, then the
table — a visible shape jump:
1. During the deferred network round-trip, `<Deferred>` rendered an
8-plain-bar fallback.
2. Once data arrived, `<TransactionList>` mounted with `isLoading`
initialized to `true` and rendered its **own, differently-shaped**
internal table skeleton for ~1 frame.
3. Then the real table.
Sequence: `plain-bars skeleton → table-shaped skeleton (~1 frame) →
table`.
## Fix
- Extract the internal table skeleton into a shared, exported
`TransactionListSkeleton`.
- Use it for **both** the `<Deferred>` fallback (`Accounts/Show.tsx`)
**and** `TransactionList`'s own `isLoading` branch — one consistent
skeleton, zero duplication.
- Initialize `isLoading` from `providedTransactions === undefined`, so
the redundant loading frame is dropped when data is already present at
mount.
### Before → After
| Phase | Before | After |
|-------|--------|-------|
| Deferred fetch | 8 plain bars | table-shaped skeleton |
| TransactionList mount | table-shaped skeleton (~1 frame) | (none —
data already present) |
| Loaded | table | table |
Result: a single, consistent, table-shaped loading state — no shape
jump.
## Shared-component safety
`TransactionList` is also used by the **budgets** show page
(`budgets/show.tsx`), which always passes a resolved `transactions`
array behind its own loading gate — so the `isLoading` init change keeps
that path rendering the table immediately. The transactions **index**
page has its own table and does not use `TransactionList`. Accounts with
zero transactions still render an empty table (unchanged).
## Tests
- New `transaction-list.test.tsx` asserts `TransactionListSkeleton`
renders the table-shaped skeleton (bordered container + many
placeholders), i.e. not the old plain-bars shape.
- `Accounts/Show.test.tsx` mock updated to export
`TransactionListSkeleton`; existing tests stay green.
- `bun run format`, `bun run lint` clean. No new TypeScript errors
introduced (pre-existing count unchanged).
## 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.
## Summary
Follow-up to #623 — addresses security findings **not covered** by the
maintainer's draft PR #627. The original scope was larger; out-of-scope
changes were reverted (see `revert: drop out-of-scope changes from
security round 2`) and are **no longer part of this PR** (details under
_Reverted / not included_).
## Changes
### Rate limiting
- `routes/api.php`: authenticated API group throttled at `300,1` (per
user). An SPA with offline sync + multi-widget dashboards fans out many
requests per interaction, so a tighter cap throttled legitimate bursts.
- `routes/web.php`: unauthenticated open-banking callback throttled at
`30,1` (per IP), enough headroom for browser retries and the iOS PWA →
Safari hand-off that can fire the callback twice.
- `use-decrypt-transactions.ts`: the legacy decrypt migration loops
without backoff (~3 requests per 100 encrypted transactions), so a large
account could hit the API throttle and abort the whole migration
silently. It now honours Laravel's `Retry-After` header on 429 and
retries the same request instead of bailing.
### State token persistence on failure
- `AuthorizationController::callback`: clears `state_token` on the
connection when EnableBanking session creation fails, preventing a stale
token from being replayed.
### Trust proxies hardening
- `bootstrap/app.php`: replaced `trustProxies(at: '*')` with an
env-based `TRUSTED_PROXIES` config. When unset, no proxy is trusted
(Laravel default) instead of trusting every IP.
- ⚠️ **Deploy requirement:** production runs behind a reverse proxy /
TLS terminator (`Dockerfile.production` + nginx serve HTTP on :80). With
`TRUSTED_PROXIES` unset, Laravel stops trusting
`X-Forwarded-For`/`X-Forwarded-Proto` → client IPs collapse to the proxy
IP (contaminating logs and per-IP throttling) and HTTPS detection
breaks. `TRUSTED_PROXIES` **must** be set in the production environment
before merge, and should be added to `.env.production.example`.
### XSS-safe Blade-to-JS injection
- `app.blade.php`: replaced `{{ }}` Blade syntax inside `<script>` with
`@json()` to prevent JS injection via variable content.
### TOCTOU defense-in-depth
- `Api/TransactionController::bulkUpdate`: added `->where('user_id',
$userId)` to the per-row UPDATE as belt-and-suspenders behind the
existing `abort(403)` ownership pre-check.
## Reverted / not included
The following were in the original description but were reverted and are
**not** in the current diff:
- `block-demo` middleware on additional route groups (already present
upstream on `routes/settings.php`).
- `->limit(500)` / `has_more` / `since` validation on
`TransactionSyncController`.
## Notes
- `vite.config.ts` appears in the diff only because of an older
merge-base; it is identical to `main` and nets to no change (will vanish
on rebase).
- No tests yet for the `state_token` clearing on `createSession` failure
or for the throttles — worth adding before merge (the callback throttle
is the most exposed surface).
---------
Co-authored-by: Víctor Falcón <victoor89@gmail.com>
## Summary
Migrates the last two flows off the deprecated client-side
IndexedDB/Dexie "offline-first" model so they read from the backend
instead of the local database. The Dexie cache was already being
dismantled and is no longer the source of truth for the transaction
list; these two consumers were the last ones still tied to it, and the
local read could silently return incomplete data.
**Duplicate detection on import** and **the account detail transaction
list** now both come from the server.
### What changed
- **Import duplicate detection** → new `POST
/api/transactions/check-duplicates`. Matches on day + amount (integer
cents) + normalized description server-side, replacing
`sync()`-then-read-Dexie, which silently missed duplicates that fell
outside the local cache window.
- **Account detail transaction list** → served as an Inertia prop from
`AccountController@show` (eager-loading `category` + `labels`) instead
of fetching every transaction from `/api/sync/transactions` and
filtering client-side (which could show an incomplete list for accounts
with long history).
- **Import preview "existing transactions" panel** → reads from `GET
/api/transactions?account_id=…&per_page=10` (the index gains an
`account_id` filter + clamped `per_page`) instead of Dexie.
- `TransactionList` now renders only from the provided prop; the
`/api/sync` fetch path, the Dexie-backed search, and the `refreshKey`
refetch are removed (search is in-memory; create triggers a scoped
`router.reload({ only: ['transactions'] })`).
### Commits
1. `feat` — the backend migration (endpoints, controller prop, frontend
rewiring, tests).
2. `refactor` — centralize the "has transaction ledger" rule in
`AccountType::hasTransactionLedger()` (a single named concept the
frontend mirrors, documented as distinct from `isNonTransactional()`),
and add an `id` tiebreak for deterministic pagination.
3. `fix` — drop rows reassigned to another account from the account list
(they lingered now that the list no longer refetches).
4. `fix` — match Unicode whitespace (e.g. non-breaking spaces) in
server-side duplicate detection, matching the old JS behavior; hardens
endpoint tests.
## Review notes
Two review agents (architecture/quality and bugs/correctness) reviewed
the change. Applied: the `AccountType` single-source-of-truth, stable
pagination sort, the account-reassignment stale-row fix, the
Unicode-whitespace parity fix, and extra edge-case tests (validation,
date-range boundary, cross-user scoping).
**Deferred (follow-up):** `AccountController@show` still loads the
account's full transaction history into the Inertia payload. It's
acceptable now (single account, strictly better than the old
load-all-accounts approach) and marked with a `ponytail:` comment, but
accounts with very long history should move to a deferred +
cursor-paginated prop (the new `?account_id=` endpoint can back it).
Also, the deprecated Dexie/sync-manager stack (`sync()`, `getAll`,
`getByAccountId`, `/api/sync/transactions`) now has fewer callers and
can be retired in a dedicated cleanup.
## Test plan
- `tests/Feature/Api/TransactionDuplicateCheckTest.php` — matching,
normalization (incl. NBSP), date-range boundary, account scoping, IDOR,
validation.
- `tests/Feature/Api/TransactionIndexTest.php` — `account_id` filter +
newest-first order, `per_page` cap and lower clamp, cross-user scoping.
- `tests/Feature/AccountControllerTest.php` — the account-show
`transactions` prop (own vs other accounts, non-transactional accounts).
- Existing `DecryptTransactionsTest` (the other `/api/transactions`
consumer) unaffected.
- Frontend vitest suites for the touched areas pass.
Draft while the deferred payload/cleanup items are discussed.
## What
Migrate the Vite source map upload from the decommissioned Bugsink
instance to Sentry. Bugsink is no longer used error tracking is fully on
Sentry.
- Point `sentryVitePlugin` at the Sentry EU region
(`https://de.sentry.io/`), org `whisper-money`, project `php-laravel`.
- Read the auth token from `process.env.SENTRY_AUTH_TOKEN` instead of
hardcoding it.
- Remove the hardcoded Bugsink URL, org, project, and auth token.
Source maps go to `php-laravel` (not `frontend`) because frontend JS
errors are captured there app.tsx initializes Sentry with
`SENTRY_LARAVEL_DSN`, and the live JS issues (e.g. `PHP-LARAVEL-2Y`,
platform `javascript`, `assets/app-*.js` frames) confirm it.
## Source map leak fix
A single flag (`SENTRY_AUTH_TOKEN` present) now gates **both** map
generation and upload, so the "maps generated but never deleted" leak
into `public/build/assets/` can no longer happen:
- **With token** (production CI): `sourcemap: 'hidden'` → maps are
emitted, uploaded, then deleted. `'hidden'` omits the `sourceMappingURL`
comment, so browsers never fetch them even in the window before
deletion.
- **Without token** (local/dev/CI): `sourcemap: false` → no maps
emitted. Nothing to leak, nothing to upload.
## Follow-ups (outside this file)
- [ ] **Rotate the old Bugsink token** (`b0f46d1b…a2e8a`) it is removed
here but remains in git history.
- [ ] **Provide `SENTRY_AUTH_TOKEN` to the production build** so uploads
actually run. `Dockerfile.production` runs `bun run build:ssr` without
it today; inject it via a BuildKit `--mount=type=secret` (not an `ARG`,
which persists in image history). Until then uploads are skipped safely,
but frontend stacks stay minified.
## Summary
Starts from a security finding on the background-job status endpoints
and folds in the most important issues surfaced by a follow-up review of
the same feature area (categorization backfill, bulk rule re-evaluation,
automation-rule apply).
Each change is its own commit.
## Changes
### 1. `fix(security)` — scope job-status cache keys to the owning user
The categorization, bulk re-evaluation, and apply status endpoints
looked jobs up by a bare job UUID with no user scoping. Any
authenticated user who obtained another user's job id could poll its
progress payload. Cache keys now include the owning user's id, so a
status request keyed by the polling user's id resolves only that user's
own jobs — a mismatched owner falls through to the existing 404. No
ownership store or extra lookup. Cross-user isolation tests added for
all three endpoints.
### 2. `fix(automation-rules)` — re-check `only_uncategorized` at apply
time
The apply flow cached a match snapshot for up to 15 min (keyed only by
`rule.updated_at`) and applied the rule's category to every id in it
without re-checking eligibility. A transaction categorized *after* the
snapshot (by the user, a sibling rule, or a concurrent AI backfill) was
silently re-categorized and stamped `category_source=Rule`. Now
re-filtered through `shouldSkipForOnlyUncategorized()` at apply time;
skipped rows are no longer counted as changed.
### 3. `perf(automation-rules)` — memoize rule set per user in
`applyRules()`
Bulk re-evaluation calls `applyRules()` once per transaction, and each
call re-queried the user's whole rule set + labels — an N+1 scaling with
transaction count (the sibling apply job already loaded rules once).
Memoized per user for the service instance's lifetime (resolved fresh
per job, so rules created mid-run are intentionally not seen).
Query-count test asserts one `automation_rules` query regardless of
transaction count.
### 4. `fix(transactions)` — stop polling when the job never starts
The re-evaluate and apply pollers rescheduled on any non-terminal
status, including `pending`. If the queue worker is down the job never
runs, `failed()` never fires, and the client polled for the full
hour-long TTL with a stuck spinner. Now gives up after 30 consecutive
`pending` ticks (~30s), mirroring the guard the AI-categorization poller
already has. Long `processing` runs are unaffected.
### 5. `test(automation-rules)` — cover apply job execution and failure
branches
`ApplySingleAutomationRuleJob` was only asserted to be *pushed*; its
`handle()` body and `failed()` branch had no coverage, and
`ReEvaluateTransactionRulesJob::failed()` was untested too. Added direct
`handle()`/`failed()` tests pinning the progress-cache payloads.
## Deferred follow-ups (surfaced by review, not in this PR)
- **Consolidate the three frontend pollers onto `usePollJobStatus`** —
they hand-roll `setTimeout` loops and don't tear down on unmount (can
`setState`/`onClose` after the dialog closes). The hook already exists
and is unmount-safe; routing all three through it would also make the
`pending` cap unit-testable. The cap in change #4 currently mirrors the
already-shipped AI poller and is not separately unit-tested.
- **`noteAlreadyPresent()` uses a substring match**
(`AutomationRuleService`) — a rule note that is a substring of an
existing note is silently not appended. Should compare note lines
exactly.
- **No dedup/lock on concurrent apply/re-evaluate jobs** — note appends
are a non-atomic read-then-write, so two concurrent jobs can duplicate a
note. Category/label writes are already idempotent.
- **Transient 404 → false "failed"** — a mid-run cache
eviction/TTL-expiry (or the deploy window of change #1, where an
in-flight job's pre-deploy key is orphaned) makes the poller surface a
false failure. Retry a few times on transient errors before giving up.
- **`matches()` pagination** advances `next_offset` by fetched rows, not
page-window size — deleted ids can stall infinite scroll before reaching
`total`.
- **"Apply to N" count / encrypted-skip counts** can be stale or
under-report vs what's actually processed.
- **Job-trait convention drift** between the three jobs (modern
`Queueable` vs legacy trait stack).
## Testing
- `vendor/bin/pint --test` — pass
- `bun run format` / `bun run lint` — pass (one pre-existing unrelated
warning in `chart.tsx`)
- Affected Pest suites (apply, re-evaluate, evaluation, rule,
categorization, apply-rule-suggestions) — 94 tests pass
## What
`demo:reset` now bails out early with an info message when
`config('app.demo.enabled')` is falsy, instead of rebuilding the demo
account regardless of the `DEMO_ENABLED` flag.
## Why
The `DEMO_ENABLED` config existed but the command ignored it, so a
scheduled reset would recreate demo data on environments where the demo
account is turned off.
## Problem
`SendAiConsentFollowUpEmailsCommandTest > it treats consent exactly
three days after signup as onboarding` fails intermittently on `main`
(more likely under parallel load):
```
The unexpected [App\Jobs\Drip\SendAiConsentFollowUpEmailJob] job was dispatched.
```
## Root cause
The `userWithConsent()` helper builds two timestamps from separate
`now()` calls:
```php
User::factory()->create(['created_at' => now()->subDays($signedUpDaysAgo)]); // now() at T1
$user->aiConsents()->create(['accepted_at' => now()->subDays($acceptedDaysAgo)]); // now() at T2 > T1
```
The command treats consent given more than `ONBOARDING_GRACE_DAYS` (3)
after signup as a post-onboarding opt-in:
```php
if ($consent->accepted_at->lessThanOrEqualTo($user->created_at->copy()->addDays(3))) {
continue; // skip
}
```
For the boundary case (`signedUpDaysAgo: 5, acceptedDaysAgo: 2`),
`created_at + 3 days = T1 - 2 days` and `accepted_at = T2 - 2 days`.
Because `T2 > T1` by microseconds, `accepted_at` lands just *after* the
grace boundary, the `<=` returns `false`, and the job is dispatched —
failing the assertion.
## Fix
Freeze time for the test file so all day-relative timestamps land on
exact boundaries. The command logic is correct and unchanged.
## Testing
```
php artisan test tests/Feature/SendAiConsentFollowUpEmailsCommandTest.php
# 9 passed
```
> **Draft on purpose — a partial fix + a design for the full one.**
These commits are safe, behavior-preserving reductions of the N+1, but
they do **not** fully resolve PHP-LARAVEL-40. The complete fix is a
batch-aware refactor of the AI rule-learning path, which is delicate
(getting it wrong silently mis-categorizes users' transactions). I've
written the design below for a human to implement and diff against the
current per-transaction behavior. Current user impact is low (~170 ms
request, 1 event), so there's no urgency to rush the risky part.
## What
Sentry **PHP-LARAVEL-40** — N+1 in `TransactionController@bulkUpdate`
(PATCH `/transactions/bulk`). A bulk category update calls
`CategoryOverrideHandler::record()` once per transaction, and each call
runs the full AI rule-learning pipeline
(`AiRuleLearner::forgetFromAiRules()` + `learnFromCorrection()`):
loading all the user's AI rules, plucking every description, running
matcher `count(*)` probes, and inserting a `category_corrections` row.
For a 112-transaction batch that's ~112× each. The offending span is the
per-transaction `category_corrections` insert.
## What this branch does (safe, partial)
Two behavior-preserving reductions, each validated by the existing
learning tests:
1. `perf(transactions): resolve the override handler once for bulk
updates` — `bulkUpdate()` re-resolved `CategoryOverrideHandler` from the
container every iteration. Resolve it once (also required for #2 to take
effect).
2. `perf(ai): memoize the description corpus per user in AiRuleLearner`
— `learnFromCorrection` reloaded and re-tokenized every one of the
user's descriptions on each transaction. The corpus is immutable while
only categories change, so memoize the document-frequency map + count
per user. Removes one `SELECT` (and its tokenization) per transaction.
3. `test(ai): assert batch corrections learn correctly through the
memoized corpus` — proves the second, memoized-corpus learning still
yields a correct, distinct clause (not just that the query count
dropped).
4. `test(ai): guard AiRuleLearner against a singleton binding` — the
cache has no invalidation and is only safe while the learner is resolved
fresh per request; a test now fails if it is ever bound singleton.
## What it does NOT do
It does **not** resolve the flagged N+1. Still per-transaction: the
`category_corrections` insert, `forgetFromAiRules`' reload of all AI
rules, the matcher `total()`/`countMatchingAll()` overbroad probes,
`releaseClauseFromOtherCorrectionRules`, and `existingCorrectionRule`. A
learnable transaction still costs ~6–10 queries. Treat PHP-LARAVEL-40 as
**reduced, not closed** (hence no `Fixes` keyword).
## Reviewed by two independent agents (architecture +
product/correctness)
Both verified the two perf commits are **behavior-preserving and safe**:
the memo cannot go stale (nothing in the correction path mutates
descriptions; the bulk `update()` only writes
`category_id`/`category_source`/`ai_confidence`/`categorized_by_rule_id`,
and runs after the loop), no cross-user leak (keyed by user_id; learner
is transient, one user per request, no Octane), and the handler is
stateless. The existing 34 learning tests exercise the corpus→rule
computation on fresh loads.
## Proposed full fix (for a human to implement)
Exploit that a bulk update sends **all transactions to the same category
for one user.** Add `CategoryOverrideHandler::recordBulk(Collection,
?string)` backed by `AiRuleLearner::learnFromCorrections(array,
string)`; load user-level data once, mutate in memory, write once:
1. **Batch-resolve** `categorized_by_rule_id` via one `whereIn` instead
of per-txn `find()`; apply the **identical** per-txn learnable test
against the map.
2. **Batch-insert** corrections: snapshot each AI-driven txn's *old*
`from_category_id`/`source`/`confidence` during the loop, collect rows,
one insert. (Note: raw `insert()` skips model events/UUID/timestamps —
verify `CategoryCorrection` has no `creating` hook, else chunked
`create` in one transaction.)
3. **Forget once**: union all learnable txns' merchant tokens, load AI
rules once, apply the **whole union** to each rule *before* the
delete-vs-save decision, save/delete each once.
4. **Learn once**: compute each clause (merchant or memoized-corpus
tokens), dedupe within the batch and against the target rule, run
`releaseClauseFromOtherCorrectionRules` for the union once, load/create
the single target correction rule once, append all, save once.
5. Wrap 2–4 in one `DB::transaction`, and route the single-txn path
through the same method (one-element collection) so the two can't drift.
**Correctness invariants to preserve (call these out in review):**
- Apply-all-then-decide for both `forget` and `releaseClause` deletes —
an incremental forget/save/forget would delete a rule a later txn's
clause should have kept.
- Corrections must read each txn's **old** category/source/confidence —
run before the bulk `update()`, as today.
- Clause dedup must match `appendClause`'s loose `==` array comparison.
- Validate with a golden-set test diffing learned/forgotten rules
before/after against the current per-txn path over a mixed batch
(merchant txns, description txns, encrypted/no-merchant skips,
re-corrections that move a key between categories).
## Testing
- `tests/Feature/Ai/` + `BulkUpdateTransactionsTest` + IDOR/decrypt:
153/153 green. `pint` clean.
Refs PHP-LARAVEL-40
## What
Addresses Sentry **PHP-LARAVEL-3X** — a slow DB query in
`App\Jobs\SendDailyBankTransactionsSyncedEmailJob::handle()`.
The daily "transactions synced" email filters transactions by:
```php
Transaction::query()
->where('user_id', $this->user->id)
->where('source', TransactionSource::EnableBanking)
->when($lastSentMailLog?->sent_at, fn ($q, $at) => $q->where('created_at', '>', $at))
->whereHas('account.bankingConnection', ...) // EXISTS on PKs
->get();
```
The only `user_id`-leading index is `idx_transactions_budget_lookup
(user_id, transaction_date, category_id)` — its second column is
`transaction_date`, **not** `created_at`, so it can't serve
`source`/`created_at`.
## How
Add a composite index matching the predicate — two equalities then the
range:
```
idx_transactions_user_source_created (user_id, source, created_at)
```
The `whereHas` compiles to correlated `EXISTS` subqueries that join on
primary keys (`accounts.id`, `banking_connections.id`), so no extra
index on those tables is needed — the `transactions` index alone gives
the optimizer the selective driving path it currently lacks.
## Production verification (via read-only prod queries)
Confirmed the diagnosis and de-risked the deploy against the live
database:
- **The query is genuinely slow.** `EXPLAIN ANALYZE` for the heaviest
user (~6.1k EnableBanking transactions) runs in **~6 s**. The optimizer,
lacking a selective path, drives from a **full table scan of all ~2,500
accounts** and examines ~108k transaction rows (334 account loops × ~323
rows), filtering `user_id`/`source` only afterwards.
- **The index fixes it.** `(user_id, source, created_at)` lets the
optimizer drive from transactions — a seek to `(user_id,
'enablebanking')` (~6.1k rows) plus primary-key semi-joins — far cheaper
than the current ~108k-row plan, so it will be chosen.
- **The deploy is low-risk.** `transactions` holds **~297k rows** (not
millions). Adding a secondary index on InnoDB/MySQL 8 is online
(`ALGORITHM=INPLACE, LOCK=NONE`, no table rebuild), so at this size the
build is seconds and does not block the sync write path.
## Commits
1. `perf(db): index transactions for the daily synced-email query` — the
migration + a test asserting the index columns/order.
2. `test(db): assert the daily-email index name, not just its columns` —
lock the explicit index name (review feedback).
## Reviewed by two independent agents (architecture +
product/operational risk)
Both rated the change correct and shippable: column order right
(equalities before the range), migration reversible, index non-redundant
(an existing index can't be widened without breaking budget queries),
and no behavior/result change (a secondary btree index never alters
result sets; the job has no `ORDER BY`).
## Impact / risk
- **User impact:** indirect — a background email job (Sentry reports 0
interactive users), but the ~6 s query wastes queue-worker time and IO
on every run.
- **Complexity:** low — one additive, reversible index migration; no
application logic changed.
- **Write path:** one more index maintained per transaction insert (10th
on the table). Marginal, consistent with the existing UUID-leading index
cost profile.
Fixes PHP-LARAVEL-3X
## What
Fixes the N+1 flagged by Sentry **PHP-LARAVEL-3Y** in
`App\Jobs\SyncBankingConnectionJob`.
`TransactionSyncService::importTransaction()` ran one `exists()` probe
**per incoming bank transaction** to detect duplicates:
```sql
select exists(
select * from transactions
where account_id = ?
and (dedup_fingerprint = ? or external_transaction_id = ?)
) as exists
```
A sync importing N transactions issued N dedup `SELECT`s. As transaction
volume per sync grows, so does the query count.
## How
Preload the account's existing dedup keys **once** at the start of
`sync()` and match against in-memory sets:
- `loadExistingDedupKeys()` streams (`cursor()`) the two dedup columns
for the account — including soft-deleted rows (`withTrashed()`) — into
two keyed sets. One query instead of N.
- `importTransaction()` checks membership in memory (`isset()`), an
exact mirror of the old `fingerprint OR external_id` predicate.
- Keys from successful inserts are folded back into the sets, so
duplicates **within the same sync** (e.g. the same transaction repeated
across pages) are still caught in memory.
- The `(account_id, dedup_fingerprint)` unique index +
`UniqueConstraintViolationException` catch still backstop concurrent
syncs (`SyncBankingConnectionJob` is already `ShouldBeUnique` per
connection).
## Commits
1. `perf(banking): preload dedup keys to kill per-transaction N+1` — the
core fix + a query-count regression test.
2. `perf(banking): stream dedup key preload with a cursor` — avoids
buffering every historical row into a Collection on top of the sets
(memory guard for very large accounts).
3. `fix(banking): match external ids case-insensitively in dedup` — the
old query compared `external_transaction_id` under the production
`utf8mb4_unicode_ci` collation; the in-memory `isset()` is byte-exact.
With no unique index on `external_transaction_id`, a legacy id stored as
`ABC` vs an incoming `abc` would have silently double-imported.
Normalized with `mb_strtolower()` on both sides + a test.
4. `test(banking): cover intra-run cross-page dedup` — new coverage for
the same transaction appearing on two pages of one sync.
## Testing
- 303/303 in `tests/Feature/OpenBanking/` green; full backend suite
green except one **pre-existing, unrelated** failure on `main`
(`DashboardTest` returns 409 locally — an Inertia asset-version artifact
that resolves once `bun run build` runs in CI).
- New tests: dedup runs as a single preload SELECT regardless of batch
size; case-insensitive external-id dedup; cross-page intra-run dedup.
- `pint` clean; `larastan` clean on the changed file.
## Reviewed by two independent agents (architecture +
product/correctness)
Both rated the change behavior-preserving and shippable. Follow-ups they
surfaced, intentionally **not** bundled here to keep this fix focused
and low-risk:
- **`WiseTransactionSyncService` has the identical latent N+1**
(`:149-158`). It is not currently firing in Sentry (lower volume). Worth
a separate PR — possibly extracting a shared dedup-set helper once both
call sites need it.
- **Residual collation divergence**: accent/width folding from
`utf8mb4_unicode_ci` is not replicated in PHP. Irrelevant for real bank
reference ids (ASCII), accepted.
- The test comment referencing a "cleanup command" that repoints orphan
fingerprinted rows describes a command that does not exist in the repo —
pre-existing, worth correcting separately.
Fixes PHP-LARAVEL-3Y
Víctor Falcón
Anshul Nitin