Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .claude/agent-memory/e2e-test-engineer/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@
- [story-1879-report-wizard.md](story-1879-report-wizard.md) — Bank Report Wizard POM/spec; Blocker bug #1886 (budgetSources envelope crash blocks all progress past step 1) + compile errors/missing i18n keys found via `tsc`; source-report E2E seeding pattern (WI budget → invoice-budget-line link); Story #1899 added a 5th step ("Settings" — report language + moved toggles), preview iframe only exists on step 5 now.
- [story-1891-wizard-followup.md](story-1891-wizard-followup.md) — expandable invoice rows, CSP `blob:` frame-src hardened preview check (SUPERSEDED TWICE — `page.frames()` proof, then in-page blob fetch → now header+console-only, see general-e2e-patterns.md), deposit budget-source tagging; 2 filed bugs (#1892 full-exclusion display, #1893 missing deposit-default heuristic); sandbox CAN now build+boot the full container stack (see `sandbox-live-verification.md`) but browser binary download is still network-policy-blocked.
- [sandbox-live-verification.md](sandbox-live-verification.md) — **dhi.io build access is sandbox-dependent, re-check each session**: this session successfully built `cornerstone:e2e` and booted the full container stack (app+OIDC+proxy all healthy), a first — but Playwright's own browser binary download (`playwright.download.prss.microsoft.com`/`cdn.playwright.dev`) is blocked by network policy, and Ubuntu's `chromium-browser` apt package is a non-functional snap stub (no snapd) — no way found yet to get an actual live browser run in this sandbox class.
- [story-1900-editable-report-preview.md](story-1900-editable-report-preview.md) — step 5 reworked from always-present auto-regenerating iframe → live editable surface + on-demand PDF Modal; POM rename `waitForPreviewReady/Regenerated` → `openPdfPreviewModal`/`closePdfPreviewModal` (must close before another modal-opening action); 4 filed bugs (#1904-#1907, all now FIXED+CLOSED, see file's re-verification section); deliberate deviation editing a file explicitly marked "do not touch" because leaving it broke `tsc` workspace-wide — see file for the general lesson; `footnoteFetchFailed` skip note naturally reachable with no Paperless container, no mock needed; re-verification added `mobileCard()`/`mobileUsageField()` POM locators + Scenario 15 (#1907 regression guard) + found/filed NEW bug #1908 (mobile-card fallback visible on desktop, no default `display:none`) with its own expected-to-fail Scenario 1b.

## Open follow-ups to flag to orchestrator

- No Paperless-ngx testcontainer exists yet (story-epic08-e2e.md) — all Paperless E2E coverage is `page.route()` mocked, not real integration. Add the container when Paperless work resumes.
- **Filed bugs from Story #1891, unresolved**: #1892 (fully-excluding all budget lines on an invoice removes it from the report wizard's step-3 list instead of showing €0.00 — display-only, PDF/claim unaffected) and #1893 (deposit budget-source auto-default 0/1/>1-source heuristic is entirely unimplemented — `InvoiceDepositsSection.tsx` never receives the invoice's budget lines).
- **Filed bug from Story #1900 re-verification, unresolved**: #1908 (Major — `.mobileCardList` has no default `display:none`, so the #1904 mobile-card fix also renders duplicated on desktop/tablet; one-line CSS fix, regression-guard test already in place as Scenario 1b in `reportWizardEditableContent.spec.ts`, currently expected-to-fail until fixed).
- Containerized E2E verification capability varies by sandbox instance — **do not assume the old "no dhi.io creds" note still applies**; re-attempt `docker build -t cornerstone:e2e .` each session before falling back to static-only verification. See `sandbox-live-verification.md` for the current state and the remaining browser-binary blocker.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion .claude/agent-memory/product-architect/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
- [Dual-rail aggregation](dual-rail-aggregation.md) — Rail A/B tagged-deposit invariants (#1891/PR #1894), residual-denominator rule, isSplit UNION
- [Source-report split inference](source-report-split-inference.md) — budgetLines[]/deposits[] are this-source-scoped, so †/‡ classification is a proxy; proposed `splitKind`; pdfmake `'2*'` width trap
- [Story reviews](story-reviews.md) — per-story and per-PR review log
- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation
- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation + the reportContent content/layout split (#1900)
- [Diary drafts pattern](diary-drafts-pattern.md) — ADR-022 draft lifecycle via status column on parent table
- [EPIC-03 refinement](epic03-refinement.md) — 40 consolidated refinement items
- [EPIC-04 household items](epic04-household-items.md) · [EPIC-05 budget](epic05-budget.md) · [EPIC-17 i18n](epic17-i18n.md) · [EPIC-18 areas & trades](epic18-areas-trades.md)
Expand Down
57 changes: 56 additions & 1 deletion .claude/agent-memory/product-architect/client-pdf-pipeline.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ app's ambient locale**. Two locale-bound artifacts are built in `ReportWizardPag
into `generateReportPdf` -- nothing else in the pipeline is locale-aware:

- `reportT = i18n.getFixedT(reportLanguage, 'budget')` -- works synchronously because
`client/src/i18n/index.ts` statically imports *both* `en` and `de` bundles into one `resources`
`client/src/i18n/index.ts` statically imports _both_ `en` and `de` bundles into one `resources`
object. **Never call `i18n.changeLanguage()` to switch report language** -- that mutates the app UI.
- `reportFormatters = createFormatters(localeTag, currency)` in `client/src/lib/formatters.ts`.
`useFormatters()` is now a thin wrapper over the same factory, so UI and report formatting can
Expand All @@ -101,3 +101,58 @@ took `t: TFunction` + `formatters?: Formatters` params and contain **no** ambien
`reportPdf/` would silently leak the UI locale into the exported PDF.

TODO (mine): ADR-034 does not yet record this contract. Add it.

## Content/layout split: `client/src/lib/reportContent/` (Story #1900, PR #1909)

An editable content model now sits between the API response and the pdfmake builders:

```
SourceReportResponse --buildReportContent(report, includedIds, useCase, reportT, reportFormatters,
{includeCoverLetter, household})--> ReportContent (plain strings)
ReportContent + Record<string,string> overrides --applyOverrides--> effective ReportContent
effective ReportContent --generateReportPdf--> Blob
```

- `buildReportContent` owns **all** text derivation, including `includedTotal` and per-status subtotals.
`merge.ts` / `overviewPdf.ts` / `coverLetterPdf.ts` are now pure layout with zero derivation — this
finally removes the "`report.totalAmount` is not the grand total once invoices are excluded" trap from
the layout layer.
- `applyOverrides` is pure, ignores unknown keys, and **recomputes `coverLetter.signature` from `sender`**
(the one derived-field invariant). Override keys: `coverLetter.{sender,recipient,reference,subject,body}`
and `row.<invoiceId>.{usageText,attachmentsNote}`.
- Two independent references by design: `coverLetter.reference` (editable) vs `sourceInfo.referenceText`
(the overview block's read-only Reference line). Both baseline off `report.source.reference`; editing the
letter does **not** change the table.
- `ReportContentRow.status` = raw key for the `Badge` variant map; `statusText` = report-language label.
Both are needed; a consumer that renders the Badge label from its own `t()` re-introduces the UI-locale leak.

**Labels live in the model (closed on PR #1909 round 2).** `ReportContentLabels` (12 strings) is built with
`reportT` in `buildReportContent` and consumed by BOTH `ReportContentEditor` and `overviewPdf` — one
translated string set per report, so a second consumer cannot drift. Still re-translated independently
(correct today, both receive `reportT`): `overviewPdf` skip-footnote reasons, `merge.ts` page header/footer.
General rule when adding a consumer: **check which `t` it receives.**

### Report-language vs UI-language: the export test

The discriminator is **"does this exact string appear in the exported PDF?"** — NOT "is it next to an
editable control?". Ruled on PR #1909 round 2:

- `labels.usage` IS a PDF `tableHeader` (`overviewPdf.ts`) -> report language everywhere it captions that
data, including the responsive mobile card. Labeling it from chrome `t` on mobile only made the language
flip at a breakpoint.
- `labels.attachmentsNote` is NOT exported (no attachments column in the PDF) — but its desktop header
already reads from `content.labels`, so mobile must match or desktop/mobile split.
- Cover-letter `senderLabel`/`recipientLabel`/`bodyLabel` stay UI-language because `coverLetterPdf` renders
those blocks with **no label prefix at all** — they are genuinely unexported. (`Reference:`/`Subject:` DO
print in the PDF, but from separate `sourceReports.coverLetter.*` keys.) So the cover-letter card is not a
precedent for putting editable-field labels on chrome `t`.

One-line statement of the rule: **visible captions of exported data follow the report language;
screen-reader affordance sentences (`ariaLabel`, `resetAriaLabel`) are wholly UI language** — never splice a
report-language noun into a UI-language sentence. This pre-decides the same question for #1901.

**Forward note for #1901 (AI generation):** `ReportContentOverrides` has no provenance concept, and
`buildReportContent` is a closed pure derivation with no injection seam. AI text dumped into `overrides`
would show the "edited" dot on every field, offer a reset-to-non-AI-text, and be silently wiped by
`guardedUpdate` on any step 1-4 change. Design a third layer (`baseline -> generated -> user`) or a
`generatedText` parameter — do not let it default into the overrides map.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Source reports allocate an invoice to a budget source via two independent rails.

**Testable consequence:** summing `allocatedAmount` across all sources on a fully-line-covered invoice reconstructs the invoice amount exactly. Canonical fixture: 1000€ invoice, 600€ line→A, 400€ line→B, 150€ deposit tagged A ⇒ residual 0.85, A = 600×0.85+150 = 660, B = 400×0.85 = 340, Σ = 1000. Pin this invariant on any change here.

**Non-obvious corollary:** the residual fraction is *invoice-level*, so a deposit tagged to source A also reduces the Rail A share of a source-B line on the same invoice. That looks wrong at first glance; it is exactly what makes the rails reconcile.
**Non-obvious corollary:** the residual fraction is _invoice-level_, so a deposit tagged to source A also reduces the Rail A share of a source-B line on the same invoice. That looks wrong at first glance; it is exactly what makes the rails reconcile.

## `isSplit`

Expand All @@ -30,8 +30,8 @@ UNION over two arms (budget-line sources via `work_item_budgets`/`household_item
## Review heuristics that earned their keep

1. **When a function is forked into an `XExcludingY` variant, diff its core formula against the original line by line.** The residual expression was the only divergence and the only bug. ~200 lines of near-duplicate logic, one of two copies wrong.
2. **A combined-rail test that puts the two rails on *different* invoices proves nothing.** The original suite had exactly that and passed while the same-invoice case was off by 40%. Always demand the same-entity crossing case.
3. **Additive-only diffs (`@@ -N,3 +N,269 @@`, zero deletions) are strong containment** — verify with `git diff origin/beta...HEAD -- <file>` — but they only bound the blast radius to *new* code paths. They say nothing about whether the new path is correct.
2. **A combined-rail test that puts the two rails on _different_ invoices proves nothing.** The original suite had exactly that and passed while the same-invoice case was off by 40%. Always demand the same-entity crossing case.
3. **Additive-only diffs (`@@ -N,3 +N,269 @@`, zero deletions) are strong containment** — verify with `git diff origin/beta...HEAD -- <file>` — but they only bound the blast radius to _new_ code paths. They say nothing about whether the new path is correct.
4. **Tests that assert a surprising number with a long apologetic comment are a smell.** The pre-fix test literally said "1400 … is intentionally MORE than the invoice amount". That comment was the bug report.

## Open follow-ups
Expand Down
47 changes: 46 additions & 1 deletion .claude/agent-memory/product-architect/recurring-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ bug once (PR #401).

## SQLite: XOR CHECK is incompatible with ON DELETE SET NULL (bug #611)

SQLite enforces CHECK constraints *during* the FK SET NULL action. Given
SQLite enforces CHECK constraints _during_ the FK SET NULL action. Given
`CHECK((a IS NOT NULL AND b IS NULL) OR (b IS NOT NULL AND a IS NULL))` plus `ON DELETE SET NULL` on `a`,
deleting the referenced row fires SET NULL, which then violates the XOR CHECK and aborts.
**Use ON DELETE CASCADE instead.** This is why `invoice_budget_lines` (ADR-018) cascades.
Expand All @@ -47,3 +47,48 @@ Not bugs, but do not let them become the copied pattern:

- `getAllMilestones`: per-row `countLinkedWorkItems` + `getCreatedByUser`
- `sourceReportService.getSourceReport` steps d/j: per-invoice deposit fetch + per-Rail-B-invoice vendor lookup (PR #1894 M1)

## CSS Modules: `:global(.x)` never matches another module's class (PR #1909, B1)

`client/webpack.config.cjs` hashes every module class (`localIdentName: '[local]_[hash:base64:5]'` prod /
`'[name]__[local]--[hash:base64:5]'` dev). So `sharedStyles.input` renders as `input_aB3xY`, and a rule like

```css
.container :global(.input) { ... } /* DEAD -- matches a literal class "input" that never exists */
```

silently applies to nothing. It also trips stylelint's `selector-pseudo-class-no-unknown`, which is how it
surfaces (as a lint failure, masking the real defect). **The fix is always `composes:`** on a plain local
class — `composes: input from '../../styles/shared.module.css';` — then use the local class in the TSX.
That is a _different_ construct from `composes` used _inside_ a `:global` block, which is the separate,
genuinely-illegal form. `ReportContentEditor.module.css` (`composes: badge from '../Badge/Badge.module.css'`)
is the correct in-repo template.

Review heuristic: any `:global(` in a `*.module.css` referencing a class the component gets from a
`*.module.css` import is dead code. Grep for it. Corollary — CSS is untested under jsdom
(identity-obj-proxy), so a `.editedDot { opacity: 0 }` whose show-rule never matches passes a green suite
that only asserts DOM presence. Visibility assertions belong in E2E.

**The E2E guard shape that actually catches this** (`reportWizardEditableContent.spec.ts` Scenario 13, PR
#1909 round 2): resolve the token through a throwaway probe element in the page, then compare it to the
target's computed value — don't hardcode an rgb string (theme-fragile) and don't just assert "not
transparent" (passes on the page background).

```js
const probe = document.createElement('div');
probe.style.backgroundColor = 'var(--color-bg-tertiary)';
document.body.appendChild(probe);
const resolved = getComputedStyle(probe).backgroundColor; // compare against the real element
```

Ask for this whenever a CSS-Modules composition bug is the thing being fixed — it is falsifiable against a
real webpack build (class hashing included) and is the only test class that fails on dead CSS.

## Stylelint gates CI, and inline `style={{}}` escapes it

`npm run lint` = `eslint . && npm run stylelint`, and the CI `Static Analysis` job runs Stylelint as its own
step — a stylelint error is a hard beta-PR blocker. But `stylelint` only globs `client/src/**/*.css`, so
**inline `style={{ color: 'var(--nonexistent-token)' }}` in TSX is completely unchecked**. PR #1909 shipped
`var(--color-refund-text)`, a token defined nowhere; the declaration is invalid-at-computed-value and the
styling silently does nothing. When reviewing client code, grep every `var(--…)` appearing inside a `.tsx`
against `client/src/styles/tokens.css`, and push the value into a CSS Module class instead.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ metadata:

Invoice with all lines on source A + a deposit tagged to source B, no untagged deposits:
`isSplit` true, `budgetLines.length > 0`, `deposits.length === 0` → renders `†` (generic split).
AC #1898-4 says it should render `‡ reduced` — and that wording is *correct*, because the
AC #1898-4 says it should render `‡ reduced` — and that wording is _correct_, because the
invoice-level residual rule means a B-tagged deposit does reduce A's Rail A share
(see [[dual-rail-aggregation]]).

Expand All @@ -46,7 +46,7 @@ Client becomes `† iff splitKind !== 'deposits'`, `‡ iff splitKind !== 'lines
## Wiki deviation to fix (open)

`wiki/API-Contract.md` L~3610 and `shared/src/types/sourceReport.ts` both describe `budgetLines[]`
as *"all ibl lines per invoice (even portion 0)"*. Wrong — it is all of **this source's** ibl lines.
as _"all ibl lines per invoice (even portion 0)"_. Wrong — it is all of **this source's** ibl lines.
Pre-existing since #1878/#1891. Needs the correction + a Deviation Log row on API-Contract.md.

## pdfmake width gotcha (confirmed by QA on #1898)
Expand Down
27 changes: 26 additions & 1 deletion .claude/agent-memory/product-architect/story-reviews.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ APPROVED (2 medium, 3 low). Architecture verified sound; see `client-pdf-pipelin
locale-decoupling contract this established.

- **M1**: `reportLanguage` seeded once via `useState(resolvedLocale)`. `LocaleContext.syncWithServer`
sets `resolvedLocale` *asynchronously* after auth AND deletes the `locale` localStorage key when
sets `resolvedLocale` _asynchronously_ after auth AND deletes the `locale` localStorage key when
the server has a preference -- so every load starts at `detectBrowserLocale()` and flips later.
Any component seeding state from `resolvedLocale` at mount has this bug. **Reusable review check:
never `useState(resolvedLocale)` / `useState(currency)` / `useState(vatRate)` without a
Expand All @@ -224,3 +224,28 @@ locale-decoupling contract this established.
filename AC vacuously satisfied (ISO date + raw enum, nothing locale-dependent -- flag to PO).
- CI: Quality Gates green; E2E shard 5/16 red (known recurring flake in this wizard area -- must be
triaged before beta->main, not before the beta merge).

## PR #1909 — Story #1900 editable HTML report preview (REQUEST-CHANGES, 2026-07-31)

Posted as a `gh pr comment` with an explicit `**Verdict:**` line — `gh pr review --request-changes` is
rejected on a self-authored PR.

Approved as designed: the `reportContent/` content-model split (see [[client-pdf-pipeline]]), pure
`applyOverrides`, `guardedUpdate` discard flow, and the `realRender.test.ts` override→rendered-PDF chain
(the strongest test file in the repo — real pdfmake, real en/de bundles, field-isolation test).

Blockers, all in the presentation layer:

- **B1/B2** `EditableField.module.css` styled the field through `:global(.input)` — dead under hashed
CSS Modules, and 13 stylelint errors turned `Static Analysis` red. Edited-dot was permanently
`opacity: 0`. See [[recurring-patterns]].
- **B3** `ReportContentEditor` got the chrome `t` instead of `reportT` → preview in UI locale, PDF in
report language (AC #5). Also made `ReportContentRow.statusText` dead — the Badge label came from the
chrome-locale variant map instead.
- **B4** `var(--color-refund-text)` in an inline `style={{}}` — token defined nowhere, refund highlight
silently dead. Sibling `ReportInvoiceList.module.css` `.refund` is the correct pattern.
- **B5** `field: 'usage'` / `'attachmentsNote'` raw identifiers interpolated into a translated aria-label.

Lesson worth repeating: 407 green tests, and the three highest-severity defects were all "the code runs
but does nothing" — dead CSS selector, undefined token, wrong `t`. None are catchable by assertions on DOM
presence. Read the CSS and trace which `t` each consumer receives.
Loading
Loading