Larastan treats the invitedBy belongsTo as always present, so the nullsafe
access read as unnecessary. Resolve the inviter name via a scalar query keyed on
the nullable invited_by_id instead — correct when the inviter's account was
deleted (nullOnDelete) and Larastan-clean. Drops the unused invitation view key.
Address the review findings that blocked collaboration and hardened tenancy:
- Ownership validation and the view/budget/real-estate policies scope by the
active space instead of user_id, so a member can open detail pages and create
or edit data in a shared space (previously 422/403).
- Authorize purely on live space membership, dropping the current_space_id
shortcut, and reset a removed member's pointer to their personal space so no
stale-pointer write window remains.
- Apply automation rules per space, so a multi-space owner's personal-space
rules never categorise a business-space transaction.
- Flush the offline cursor before the rows on space switch to avoid a stale
re-sync. Adds member-access tests.
Add the collaboration layer for Business spaces: owners invite people by email
(seat-capped per subscription), invitees accept via a tokened link to join as
members, and owners can remove members while members can leave. Paid-feature
gates (subscription middleware, open-banking) now resolve against the active
space's owner, so a member of a Business space gets its features without a plan
of their own. Members management and invitations live on the spaces settings
page.
A user's spaces are each seeded with the same default category names, so the
per-user unique indexes on categories, labels and saved_filters collided the
moment a second space was created (500 on space creation). Rescope those
uniques to space_id (adding a plain user_id index first so the FK keeps its
index), teach the category duplicate-name handler the new index name, and
harden the creation test to seed the personal space first so it reproduces the
collision.
Flip every user-facing read (dashboard, cashflow, transactions, budgets,
accounts, settings, analytics, sync, shared props) from user-scoped to
active-space-scoped via explicit $space->… relations, and re-key the category
tree, spending, and budget-assignment services by space_id. Authorization moves
from creator-ownership to space membership.
Add the visible spaces layer behind a Pennant 'spaces' flag: a sidebar space
switcher, a settings page to create/rename/delete/switch spaces, per-space
offline-cache flushing, and the currentSpace/spaces shared props. Data creation
is unchanged — the BelongsToSpace trait already stamps space_id from the active
space. Isolation tests prove no cross-space leakage.
The provisioning pass enumerated users through the default (soft-delete-aware)
query, so trashed users got no personal space and their accounts, transactions,
categories, etc. kept a null space_id — an incomplete backfill that would block
a NOT NULL constraint and hide a restored account's data under space-scoped
reads. Include trashed users in the pass (the row-stamping already covers their
rows). Verified on a prod-scale copy: 0 rows left unstamped afterwards.
Read user_id/account_id via getAttribute so the nullable-FK guards aren't
flagged as always-true/false against the models' non-null PHPDoc types, and
backfill through the query builder (DB::table) instead of a generic Eloquent
builder, which also stamps soft-deleted rows without needing withTrashed().
Set space_id explicitly on bank-sync transaction inserts (avoids a per-row
lookup during sync and anchors the row to the account's space), and hide the
internal space_id foreign key from model serialization.
Introduce the multi-tenant Space concept without changing any behaviour yet:
every user gets an invisible personal space provisioned on creation, and all
owned models (accounts, connections, transactions, categories, labels, budgets,
automation rules, saved filters) gain a nullable space_id stamped via a
BelongsToSpace trait. An idempotent, chunked spaces:backfill command (run from a
migration) stamps existing rows so space-scoped reads can go live safely.
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
## What & why
Fixes the Sentry N+1 **PHP-LARAVEL-3V** — `N+1 insert into jobs` on
`POST /transactions`.
### Root cause
Laravel's **event discovery is on by default** (the base
`EventServiceProvider` sets `$shouldDiscoverEvents = true`, and this app
has no subclass to turn it off), so every listener in `app/Listeners` is
already wired to its event by its `handle()` type-hint.
`AppServiceProvider::boot()` was **also** registering six of them with
`Event::listen(...)`, so each ended up registered **twice** — every
queued listener published its job twice per event.
Concretely, creating a transaction dispatched
`AssignTransactionToBudget` and `CategorizeTransactionWithAi` twice each
→ repeated `insert into jobs` in one request. (The same bug made
`PostStripeEventToDiscord` post to Discord twice per webhook,
`ApplyAutomationRules` run twice, etc.)
## Changes
1. **`AppServiceProvider`** — drop the redundant explicit
`Event::listen` calls and rely on discovery, which already wires every
listener exactly once (`ScheduleDripEmailsListener` and
`SyncUserToResendListener` were discovery-only already). Verified with
`php artisan event:list`: each event now has a single listener, and the
synchronous `ApplyAutomationRules` still runs first on
`TransactionCreated`.
2. **`CategorizeTransactionWithAi`** — add `shouldQueue()` so the job
isn't even published when it would immediately no-op (already
categorized by a rule, encrypted, or user not AI-eligible — the default,
since AI is opt-in). The same guard still runs in `handle()` because
state can change while the job waits.
Together these remove the second `insert into jobs` on transaction
creation for the common (non-AI) path.
## Tests
- New `tests/Feature/Ai/CategorizeTransactionWithAiListenerTest.php`:
the AI job is queued **once** for an eligible uncategorized transaction,
and **not at all** when the user has no AI consent or the transaction is
already categorized.
- Full suite: **1828 passing**. The one failure (`DashboardTest` → 409)
is **pre-existing on `main`** (Inertia asset-version mismatch from a
local build manifest) and reproduces with these changes reverted —
unrelated to this PR.
Fixes PHP-LARAVEL-3V
## What
Removes the `AiConsentSettings` Laravel Pennant feature flag. The AI
consent management UI — the toggle in **Billing settings** and the
banner in **Transactions** — is now available to all users
automatically, no longer gated behind the flag.
The underlying AI consent functionality (model, controller, routes,
`User` consent methods) is unchanged; only the gate that hid its UI was
removed.
## Changes
- Delete `app/Features/AiConsentSettings.php`.
- `HandleInertiaRequests`: drop the `aiConsentSettings` shared prop and
its resolution.
- Frontend: render `AiConsentSection` and the transactions consent
banner unconditionally; drop the now-unused `aiConsentSettings` type and
`features` destructuring.
- Tests: remove the two flag-specific assertions in
`AiConsentSettingsTest` (consent-state coverage kept), update
`InertiaSharedDataTest`, and switch `FeatureEnableCommandTest` to
`CalculateBalancesOnImport` as its example feature.
## Testing
- `php artisan test` on the affected suites — 13 passed.
- `vendor/bin/pint`, `bun run format`, `bun run lint` — clean.
## Why
During onboarding we prompt for AI suggestions only because it's a
**paid feature** — enabling it forces the user to pick a plan at the end
of onboarding. A user who has **already connected a bank** is committed
to a paid plan regardless, so the prompt gives them nothing new. For
them we should just turn AI on.
## What
- **Connected bank → activate AI directly.** The consent prompt is
skipped and consent is recorded automatically (reuses the existing
`/ai/consent` endpoint, so the categorization backfill still runs).
`setBusy` batches with the state update so the consent screen never
flashes.
- **No bank → unchanged opt-in.** Free users still explicitly accept.
The notice is reworded to make the paid framing clear: *"AI suggestions
are a paid feature. Enable them and you'll choose a plan at the end of
the onboarding."* (translated in `es.json`).
- No backend change needed:
`RuleSuggestionController::requiresUpgrade()` already treats
bank-connected users as not needing an upgrade, and
`SubscriptionController` already gates the free plan on `!hasBank &&
!hasConsent`.
## Tests
Browser tests in `OnboardingFlowTest`:
- Connected bank → consent prompt skipped and `hasActiveAiConsent()`
becomes true.
- No bank → prompt (with the paid notice) shown; consent recorded only
after accepting.
- `/subscribe` forces a plan (no "Continue for free") when a bank is
connected or AI consent is active.
Vitest specs for `StepAiSuggestions` updated (new prop + reworded
notice) plus a new case asserting bank-connected users auto-activate.
All green: 5/5 browser tests (24 assertions), 6/6 vitest.
## What
The transactions AI consent banner now lets users decline, tells them
the outcome, and stops appearing once they've decided.
- **Dismiss option**: an X button before "Enable AI" permanently hides
the banner without granting consent.
- **Show only until the first decision**: the choice is persisted on
`users.ai_consent_prompt_dismissed_at` (set on both accept and dismiss).
The banner shows only while the user hasn't responded and never
reappears afterwards — even if they later revoke consent from settings.
- **Clear feedback**: accepting or dismissing shows a toast stating
whether AI was enabled and that the choice can be changed anytime in
**Settings > Manage Plan**.
- **Full-width layout on desktop**: the banner content spans the full
available width (button pushed to the right, text on a single line). The
narrow stacked layout is kept for mobile only.
## How
- New `POST ai/consent/dismiss` endpoint
(`AiConsentController::dismiss`).
- `User::dismissAiConsentPrompt()` (idempotent) +
`hasDismissedAiConsentPrompt()`; `store` now also marks the prompt
dismissed on accept.
- Migration adds the nullable `ai_consent_prompt_dismissed_at`
timestamp.
- `TransactionController` passes `aiConsentPromptDismissed` to the page.
## Tests
- `AiConsentTest`: idempotent dismissal, dismissal without consent, and
accept marking the prompt dismissed.
- Full consent + backfill suite and the `create a transaction` browser
test pass; pint / lint / format clean.
## Why
Feedback from onboarding users: in the categorizer step people often
**don't realize they need to categorize at least 5 transactions** before
*Continue* unlocks, and can't tell why the button is disabled. Worse,
the old counter switched to **"N remaining"** after 5 (showing *every*
uncategorized transaction, e.g. "195 remaining"), which made several
users think they had to categorize **all** of them.
## What changed
### Categorizer clarity (`step-categorize-transactions.tsx`)
- **New full-width progress banner** above the transaction card,
replacing the cramped corner counter that was getting truncated on
mobile:
- explicit instruction: *"To continue, you need to categorize at least 5
transactions."* (reuses the existing intro string)
- a **5-segment progress bar** that fills as you go
- the live **X/5** count
- a reassurance line: *"You do not need to categorize all of them."*
- When the minimum is reached (or the list is exhausted), the banner
turns **green** — *"Done! You can continue now, or keep categorizing if
you want."* — and the **Continue** button gets a ring so it's obvious
the step is unlocked.
- **Removed the misleading "N remaining"** display.
### Onboarding layout & toasts
- **Mobile top spacing** (`onboarding-layout.tsx`): reduced the large
top padding on the step content on mobile so it sits closer to the
progress dots (unchanged on desktop).
- **Toasts during onboarding** (`app.tsx`): onboarding has no bottom
navigation bar, so toasts now render **bottom-center, flush to the
bottom** instead of being lifted 110px to clear the (absent) mobile tab
bar. Behavior elsewhere in the app is unchanged.
Two new strings added to `lang/es.json`.
## Notes
- The `canContinue` / `minimumRequired` gating logic is unchanged.
- Verified: `lint` clean, `es.json` valid, `LocalizationTest` passes,
full CI green.
- Recorded mobile + desktop walkthroughs locally.
## What
Adjust the highest-index chart color variables so adjacent segments stay
distinguishable:
- **Light mode:** `--chart-9`/`--chart-10` use darker zinc shades
(`zinc-700`/`zinc-600`) instead of the near-white `zinc-100`/`zinc-50`,
which were invisible against the light background.
- **Dark mode:** `--chart-7`/`--chart-8` use lighter zinc shades
(`zinc-200`/`zinc-300`) instead of the near-black `zinc-800`/`zinc-900`.
## Why
Charts with many categories were rendering low-index and high-index
segments at nearly the same value as the background, making them
unreadable.
## Summary
Landing page copy and social proof around AI, made consistent with our
privacy stance.
- **Clarify AI as a privacy-respecting feature.** Name AI explicitly
where transactions are auto-categorized, replace the contradictory "no
AI snooping" line with an "our own AI" framing, and add an FAQ stating
the AI serves you and never trains on, sells, or shares your data.
- **Add six testimonials**, four of which highlight AI and automation
(AI categorization that learns from corrections, automatic bank sync,
the privacy framing of our own AI, and CSV auto-mapping). The other two
cover the all-in-one-place and clean-UI experience.
## Notes
- All new strings have matching `lang/es.json` translations;
`LocalizationTest` passes.
- New testimonials use placeholder gravatar hashes that 404 and fall
back to the generated `Facehash` avatar (no real photo needed).
## Test plan
- [x] `php artisan test tests/Feature/LocalizationTest.php`
- [x] Prettier formatting
## Why
On the **analysis** drawer, income/expense were derived from each
transaction's amount sign alone, ignoring the category type.
Transactions in `transfer` / `savings` / `investment` categories
therefore leaked into income/expense totals and every breakdown — e.g.
moving money between your own accounts inflated both sides. The
**cashflow** screen already keys off the category type; analysis now
does the same.
## What changed
Three commits:
1. **fix(analysis): exclude transfer, savings and investment categories
from income and expense**
Only `expense` categories (or uncategorized outflows) count as expenses,
and only `income` categories (or uncategorized inflows) count as income.
`transfer` / `savings` / `investment` are internal movements and sit on
neither side — identical to how cashflow computes income/expense/net.
2. **refactor(analytics): de-duplicate currency conversion and
category-type helpers**
`convertTransactionAmount()` / `preloadExchangeRates()` were
byte-identical in three controllers, and `categoryType()` in two.
Extracted the currency helpers into a shared
`Api\Concerns\ConvertsTransactionCurrency` trait (following the existing
`OpenBanking/Concerns` pattern) and moved `categoryType()` onto the
`Transaction` model. No behavior change.
3. **fix(analysis): net refunds within a category so totals reconcile
with cashflow**
Classification keyed off each transaction's own sign dropped contra-sign
rows entirely, so a refund booked to an expense category disappeared
instead of netting the spend down — and analysis disagreed with cashflow
on the same data (a `-10000` charge + `3000` refund read as `10000`
spent, not `7000`). A transaction's side is now decided by its category
type and signed amounts are summed before clamping, mirroring cashflow's
reconciliation across summary, category, payee, account, tag and
over-time. The largest-expenses list still shows only genuine outflows.
## Tests
Added analysis coverage for: transfer/savings/investment exclusion
(summary, breakdowns, largest, over-time), refund netting (asserts
parity with cashflow's `7000`), income-category reversals,
foreign-currency conversion, and uncategorized inflows. Full non-browser
suite green (1823 passed); `pint --test`, `bun run format`, `bun run
lint` all clean.
## Follow-up for product (not in this PR)
On **cashflow**, savings/investment outflows are excluded from expense
but re-surfaced in a dedicated "Saved & Invested" card. The **analysis**
drawer has no equivalent surface, so money categorized as
savings/investment is now correctly excluded from spending but isn't
shown anywhere. If we want analysis to fully account for it, we should
add a small "set aside" summary there. Flagged for a product decision
rather than bundled into this bugfix.
## Problem
Users keep correcting the same transactions over and over. The AI
mislabels a merchant (e.g. supermarket → fuel), the user fixes it, and
the next near-identical transaction from that merchant gets mislabeled
the same way again. Today a correction is logged and the offending ai
rule is self-healed, but the system only *forgets* its mistake — it
never *remembers* the user's fix.
## Approach
A correction now becomes a deterministic, forward-looking
`AutomationRule` (new `RuleOrigin::Correction`). The next matching
transaction is categorized by that rule **before the model ever runs**
(`ApplyAutomationRules` is synchronous and runs ahead of AI
categorization), ending the loop. Zero model cost, instant, reuses the
existing rule engine.
**Matching key** (in order):
1. **Merchant** (`creditor_name`/`debtor_name`, exact `==`) when present
— stable even as the description varies.
2. Otherwise the **description's distinctive tokens** (`in` /
AND-of-`in`), extracted by the shared `DescriptionTokenizer` (noise
tokens dropped by document frequency, language-agnostic), **guarded**
against over-broad rules that could silently mis-file en masse. If
guarded out → nothing is learned, silently.
## Deliberate decisions (from a design walkthrough)
- **Forward-only**: never retroactively re-categorizes existing
transactions.
- **Learn only from system categorizations** (AI label, ai rule, or a
prior correction rule) — never from one-off manual filing, bank
categories, or the user's own hand-authored rules.
- **A key lives in exactly one correction rule**, so changing your mind
moves it to the new category. Correcting a transaction a prior
correction rule categorized is also learnable, so correction rules stay
fixable in-flow.
- Correcting to *uncategorized* learns nothing but still self-heals the
ai rule.
- **Safety net**: correction rules are visible/editable in
`settings/automation-rules` (marked with the AI sparkle, tooltip
"Learned from your correction"); the transactions table shows a toast
with an instant **Undo**.
## Review pass (two independent agents + live QA)
- **HIGH fix** (`67fc4293`): an ai rule could out-rank a freshly learned
correction and re-apply the wrong category (when the corrected
transaction was a *direct* model label with no rule id). Now every ai
rule holding the merchant is swept on correction. Regression test added.
- **Refactor** (`ca743fde`): collapsed duplicated clause-append logic;
removed a speculative unused enum helper.
- **Coverage** (`12ceb0f7`): debtor_name path, single-token description
clause, encrypted-description fail-safe.
- **Toast conflict fix** (`382c8169`, found in live QA): correcting an
AI transaction fired both the new "Learned …" toast and the pre-existing
"Transaction categorized → Automatize" prompt, which invited the user to
manually create the rule the correction had just created. Made them
mutually exclusive. Adds a browser test for the inline-correction flow.
- **Settings icon** (`f3f882b6`): correction rules now show the AI
sparkle in settings, like ai rules.
## Verified end-to-end (against a running instance)
Drove the real UI with a browser: correcting an AI-mislabeled
transaction creates the correction rule, shows the "Learned · Undo"
toast (no competing Automatize prompt), and Undo deletes the rule while
keeping the correction. Confirmed for **merchant** keys and the
**description-only** path — including that a later "practically
identical" description (different surrounding text, no merchant) is
caught by the rule, while a near-miss sharing only one distinctive token
is correctly **not** caught.
## Open question for reviewers
**`debtor_name` (P2P) as a rule key.** For incoming transfers the
merchant key falls back to the sender's name, so correcting one can
create a rule keyed on a person's name (useful for recurring transfers
from a roommate, but a possible privacy surprise; the name appears in
the rule title in settings). This matches the existing tier-2 learner's
behaviour. Keep as-is, or restrict correction rules to `creditor_name`
only? Happy to change.
## Testing
- `tests/Feature/Ai/CategoryOverrideHandlerTest.php`: merchant +
description learning, next-transaction match, over-broad rejection,
change-of-mind move, correct-to-null self-heal, the HIGH regression,
debtor_name, single-token, encrypted fail-safe.
- `tests/Browser/CategoryCorrectionLearningTest.php`: inline correction
→ toast → learned rule → undo.
- `automation-rule-title.test.tsx`: the AI sparkle shows for `ai` and
`correction`, not `user`.
- Full AI suite green (106 tests); transaction/bulk-update suites green
(62). Pint + Prettier + ESLint clean.
No new dependency, no migration (the `origin` column is a free-text
string). The feature is implicitly gated by AI categorization — with no
AI categorization there is nothing to correct and nothing is learned.
## What
The "new since last visit" highlight (#609) stored its marker in
`localStorage` (`transactions-last-visit`), so it was **per-device**:
each device kept its own last-visit timestamp and re-flagged the same
rows. This moves the marker to a per-user server column so a single
"last visit" is shared across all your devices.
## How
- New nullable `transactions_last_visited_at` column on `users` (same
pattern as `last_active_at` / `paywall_seen_at`).
- `TransactionController@index` reads the stored marker, renders the
list with the **old** value (`lastVisitAt` prop), then advances it
forward to the newest `created_at` it served.
- Frontend drops the `localStorage` read/write and just freezes the
`lastVisitAt` prop at mount; `isNewSince` per-row logic is unchanged.
## Why newest-served `created_at` and not `now()`
Advancing to `now()` would mark a back-dated synced row (old
`transaction_date`, lands on a later page that wasn't served) as seen,
so it would never be highlighted — the exact "hiding" failure the
per-row design avoids. Advancing only to what was actually served keeps
the feature's "err on showing, never hiding" stance. The marker only
ever moves forward.
## Notes
- Same filter caveat as before: opening the list with a filter applied
advances the marker using the filtered payload. Carried over from the
original `localStorage` behavior; not a regression.
- Removed the now-dead `loadLastVisit` / `saveLastVisit` /
`newestCreatedAt` helpers and their tests.
## Tests
- `NewTransactionsMarkerTest` (feature): null marker on first visit +
stores newest served; later visit sees previous marker + advances
forward; marker never moves backward.
- `new-transactions.test.ts` (`isNewSince`), Pint, ESLint, Prettier all
pass.
## What
Between visits, highlight the transactions that arrived since the user
last opened the list.
- The newest `created_at` from the initial page load is stored in
`localStorage` (`transactions-last-visit`), frozen at mount.
- Every transaction whose `created_at` is newer than that marker gets a
subtle accent on the row (left border + faint tint).
## Why per-row instead of a single divider line
The first cut drew one "Last visit" divider with new rows above it. A
code review found this is structurally wrong: the list is sorted by
`transaction_date` (when the money moved), but "new" is defined by
`created_at` (when the row was inserted). The two don't correlate — a
bank sync that imports a batch dated across several days scatters the
new rows throughout the list, so a single boundary lands at the first
older row and **hides every new row below it**, the exact miss the
feature was meant to prevent.
Per-row marking is correct regardless of sort order: no new transaction
can be hidden.
## Notes / limitations
- The marker is written **once at mount** from the initial payload. Rows
that arrive mid-visit (load-more, a sync/refresh, AI categorization) do
not advance it, so they are never recorded as "seen" before the user has
seen them. The trade-off is they may re-appear as new next visit (errs
on showing, never hiding).
- Purely client-side (localStorage), so the marker is per-device.
## Tests
- Unit tests for `isNewSince` and `newestCreatedAt`.
- `LocalizationTest`, ESLint, Prettier all pass.
## What
Remove the fixed `max-w-[90px]` cap (and `pr-1`) on the transaction date
cell so the date column sizes to its content instead of wrapping
awkwardly.
## Why
The 90px cap forced the formatted date to wrap onto two lines in the
transactions table.
Víctor Falcón
Anshul Nitin
