From b7956a7ddfe836f2eee2a9413da7b22196aed13c Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 21:31:03 +0200 Subject: [PATCH 1/8] test(reports): update tests for #1965 report PDF legend reinstatement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the old 'footnotes always empty' describe block in buildReportContent.test.ts with 6 new tests covering AC 1.1–1.5 (split flag, depositReduced flag, both flags, neither flag, deduplication, excluded invoice). Update realRender.test.ts: the footnotes assertion now expects length 2 (split + depositReduced entries) instead of empty, removes the deposit-reduced sentence from goneFootnotes, and adds a positive check that the deposit-reduced legend sentence appears in the rendered PDF content tree. Fixes #1965 Co-Authored-By: Claude qa-integration-tester --- .../reportContent/buildReportContent.test.ts | 91 +++++++++++-------- client/src/lib/reportPdf/realRender.test.ts | 22 +++-- 2 files changed, 64 insertions(+), 49 deletions(-) diff --git a/client/src/lib/reportContent/buildReportContent.test.ts b/client/src/lib/reportContent/buildReportContent.test.ts index 75e7a4eb9..95c6b059e 100644 --- a/client/src/lib/reportContent/buildReportContent.test.ts +++ b/client/src/lib/reportContent/buildReportContent.test.ts @@ -444,72 +444,83 @@ describe('buildReportContent — rows', () => { }); }); -describe('buildReportContent — footnotes (always empty; split/deposit annotations are inline)', () => { - it('produces no footnotes when invoices are split with budget lines', () => { - const inv1 = makeInvoice({ +describe('buildReportContent — footnotes (legend sentences for split/depositReduced)', () => { + it('AC 1.1 — split flag: one split invoice included produces a footnote with id "split" and the correct keys', () => { + const inv = makeInvoice({ invoiceId: 'inv-1', isSplit: true, budgetLines: [makeBudgetLine()], + deposits: [], }); - const inv2 = makeInvoice({ - invoiceId: 'inv-2', - isSplit: true, - budgetLines: [makeBudgetLine()], - }); - const report = makeReport([inv1, inv2]); - const content = buildReportContent(report, new Set(['inv-1', 'inv-2']), 'claim', t, formatters); - expect(content.footnotes).toEqual([]); - expect(content.rows.every((r) => r.isSplit)).toBe(true); - }); - - it('produces no footnotes when all invoices are unsplit', () => { - const report = makeReport([makeInvoice({ isSplit: false })]); + const report = makeReport([inv]); const content = buildReportContent(report, new Set(['inv-1']), 'claim', t, formatters); - expect(content.footnotes).toEqual([]); - expect(content.rows[0]!.isSplit).toBe(false); + expect(content.footnotes).toHaveLength(1); + expect(content.footnotes[0]!.id).toBe('split'); + expect(content.footnotes[0]!.marker).toBe('sourceReports.table.splitInlineLabel'); + expect(content.footnotes[0]!.text).toBe('sourceReports.table.splitFootnote'); }); - it('produces no footnotes for constituted (tagged) deposit — the row gets isDeposit instead', () => { - const invoice = makeInvoice({ + it('AC 1.2 — depositReduced flag: one depositReduced invoice produces a footnote with id "depositReduced"', () => { + const inv = makeInvoice({ + invoiceId: 'inv-1', isSplit: true, budgetLines: [], - deposits: [makeDeposit({ budgetSourceId: 'src-1' })], + deposits: [makeDeposit({ budgetSourceId: null })], }); - const report = makeReport([invoice], { id: 'src-1' }); + const report = makeReport([inv], { id: 'src-1' }); const content = buildReportContent(report, new Set(['inv-1']), 'claim', t, formatters); - expect(content.footnotes).toEqual([]); - expect(content.rows[0]!.isDeposit).toBe(true); + expect(content.footnotes).toHaveLength(1); + expect(content.footnotes[0]!.id).toBe('depositReduced'); }); - it('produces no footnotes when invoices have reduced (untagged) deposits — isDepositReduced is set instead', () => { - const invoice = makeInvoice({ + it('AC 1.3 — both flags: split and depositReduced present → footnotes length 2, split first, depositReduced second', () => { + const inv = makeInvoice({ + invoiceId: 'inv-1', isSplit: true, - budgetLines: [], + budgetLines: [makeBudgetLine()], deposits: [makeDeposit({ budgetSourceId: null })], }); - const report = makeReport([invoice], { id: 'src-1' }); + const report = makeReport([inv], { id: 'src-1' }); + const content = buildReportContent(report, new Set(['inv-1']), 'claim', t, formatters); + expect(content.footnotes).toHaveLength(2); + expect(content.footnotes[0]!.id).toBe('split'); + expect(content.footnotes[1]!.id).toBe('depositReduced'); + }); + + it('AC 1.4 — neither flag: normal invoice → footnotes is empty', () => { + const inv = makeInvoice({ invoiceId: 'inv-1', isSplit: false }); + const report = makeReport([inv]); const content = buildReportContent(report, new Set(['inv-1']), 'claim', t, formatters); expect(content.footnotes).toEqual([]); - expect(content.rows[0]!.isDepositReduced).toBe(true); }); - it('produces no footnotes when an invoice has both split lines and a reduced deposit', () => { - const combined = makeInvoice({ + it('AC 1.5 — deduplication: two split invoices produce only one footnote entry', () => { + const inv1 = makeInvoice({ invoiceId: 'inv-1', isSplit: true, budgetLines: [makeBudgetLine()], - deposits: [makeDeposit({ budgetSourceId: null })], }); - const report = makeReport([combined], { id: 'src-1' }); - const content = buildReportContent(report, new Set(['inv-1']), 'claim', t, formatters); - expect(content.footnotes).toEqual([]); - expect(content.rows[0]!.isSplit).toBe(true); - expect(content.rows[0]!.isDepositReduced).toBe(true); + const inv2 = makeInvoice({ + invoiceId: 'inv-2', + isSplit: true, + budgetLines: [makeBudgetLine()], + }); + const report = makeReport([inv1, inv2]); + const content = buildReportContent(report, new Set(['inv-1', 'inv-2']), 'claim', t, formatters); + expect(content.footnotes).toHaveLength(1); + expect(content.footnotes[0]!.id).toBe('split'); }); - it('produces no footnotes when no invoice is split or has a reduced deposit', () => { - const report = makeReport([makeInvoice()]); - const content = buildReportContent(report, new Set(['inv-1']), 'claim', t, formatters); + it('excluded split invoice does not contribute to footnotes', () => { + const excluded = makeInvoice({ + invoiceId: 'inv-excluded', + isSplit: true, + budgetLines: [makeBudgetLine()], + deposits: [makeDeposit({ budgetSourceId: null })], + }); + const included = makeInvoice({ invoiceId: 'inv-included', isSplit: false }); + const report = makeReport([excluded, included], { id: 'src-1' }); + const content = buildReportContent(report, new Set(['inv-included']), 'claim', t, formatters); expect(content.footnotes).toEqual([]); }); }); diff --git a/client/src/lib/reportPdf/realRender.test.ts b/client/src/lib/reportPdf/realRender.test.ts index ac155f940..df779fafe 100644 --- a/client/src/lib/reportPdf/realRender.test.ts +++ b/client/src/lib/reportPdf/realRender.test.ts @@ -976,10 +976,8 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { depositReducedLabel: ' (less\u00A0deposit)', splitLabel: ' (partial)', // The footnote WORDINGS that must no longer appear anywhere in the tree. - goneFootnotes: [ - 'This position reflects deposits claimed separately.', - 'This is a deposit', - ], + goneFootnotes: ['This is a deposit'], + depositFootnoteText: 'This position reflects deposits claimed separately.', }, ], [ @@ -991,10 +989,9 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { // DE wrapped as "(Teilbetrag) (abzgl." / "Abschlag)" before the fix. depositReducedLabel: ' (abzgl.\u00A0Abschlag)', splitLabel: ' (Teilbetrag)', - goneFootnotes: [ + goneFootnotes: ['Dies ist eine Abschlagszahlung'], + depositFootnoteText: 'Diese Position berücksichtigt separat eingereichte Abschlagszahlungen.', - 'Dies ist eine Abschlagszahlung', - ], }, ], ] as const) { @@ -1011,8 +1008,12 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { const reducedRow = content.rows.find((r) => r.invoiceId === 'inv-deposit-reduced')!; expect(reducedRow.isDeposit).toBe(false); expect(reducedRow.isDepositReduced).toBe(true); - // #1959: the footnote array is empty — the annotations no longer live there at all. - expect(content.footnotes).toEqual([]); + // #1965: legend sentences are reinstated — split and depositReduced each produce one entry. + expect(content.footnotes).toHaveLength(2); + expect(content.footnotes[0]!.id).toBe('split'); + expect(content.footnotes[1]!.id).toBe('depositReduced'); + expect(content.footnotes[0]!.marker).not.toContain('†'); + expect(content.footnotes[1]!.marker).not.toContain('‡'); const pdfContent = buildOverviewContent(content, new Map(), t); const tableItem = pdfContent.find( @@ -1050,6 +1051,9 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { for (const gone of expected.goneFootnotes) { expect(allStrings.some((s) => s.includes(gone))).toBe(false); } + // #1965: the deposit-reduced legend sentence IS present in the rendered content. + const depositFootnoteText = expected.depositFootnoteText; + expect(allStrings.some((s) => s.includes(depositFootnoteText))).toBe(true); } }); From 610303ac7eba5692af4e81e3e6bc40139d1547de Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 21:38:22 +0200 Subject: [PATCH 2/8] feat(reports): reinstate partial and deposit-reduced legend sentences (#1965) Push split/depositReduced footnote entries from buildReportContent when the corresponding invoice sets are non-empty; preview and PDF consume them automatically. Fixes #1965 Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester --- .../story-1965-report-legend.md | 35 +++++++++++++++++++ .../lib/reportContent/buildReportContent.ts | 16 +++++++++ 2 files changed, 51 insertions(+) create mode 100644 .claude/agent-memory/qa-integration-tester/story-1965-report-legend.md diff --git a/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md b/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md new file mode 100644 index 000000000..390855128 --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md @@ -0,0 +1,35 @@ +--- +name: story-1965-report-legend +description: #1965 — PDF legend footnotes reinstated; locale-aware positive check pattern; goneFootnotes loop tail extension +metadata: + type: project +--- + +Story #1965 reinstated legend sentences for split and depositReduced footnotes in +`buildReportContent.ts` after they were removed in #1959. + +**Key changes (2026-08-03):** +- `buildReportContent.ts` now pushes `{id:'split', marker:..., text:...}` to `footnotes[]` + when `splitInvoiceIds.size > 0`, and `{id:'depositReduced'...}` when + `depositReducedInvoiceIds.size > 0`. + +**Test pattern learned — locale-aware positive check inside a locale for-loop:** +When adding a positive "this text IS present" assertion inside an `[en, de]` locale for-loop, +use `expected.depositFootnoteText` (locale-specific) rather than a hardcoded English string. +Task instructions may provide the English literal — adapt to `expected.` to avoid +a DE-iteration failure. Adding a field to each entry of the `as const` locale array is the +correct fix. + +**goneFootnotes extension pattern:** +To assert a text was REMOVED from goneFootnotes AND add a positive check it IS now present: +1. Remove the string from `goneFootnotes` array +2. Add `depositFootnoteText: ''` to the `expected` object for each locale +3. Right after the goneFootnotes inner-for loop, add: + ```ts + const depositFootnoteText = expected.depositFootnoteText; + expect(allStrings.some((s) => s.includes(depositFootnoteText))).toBe(true); + ``` + +**Coverage result:** `buildReportContent.ts` — 100% statements, 97.5% branches (uncovered: +`if (options?.includeCoverLetter ?? false)` null-coalescence truthy path, not reachable from the +new footnote tests). diff --git a/client/src/lib/reportContent/buildReportContent.ts b/client/src/lib/reportContent/buildReportContent.ts index 811da696a..77f79a60d 100644 --- a/client/src/lib/reportContent/buildReportContent.ts +++ b/client/src/lib/reportContent/buildReportContent.ts @@ -231,6 +231,22 @@ export function buildReportContent( const footnotes: ReportContentFootnote[] = []; + // Legend footnotes: one sentence per flag, deduplicated by set membership (AC 1.1–1.5) + if (splitInvoiceIds.size > 0) { + footnotes.push({ + id: 'split', + marker: reportT('sourceReports.table.splitInlineLabel'), + text: reportT('sourceReports.table.splitFootnote'), + }); + } + if (depositReducedInvoiceIds.size > 0) { + footnotes.push({ + id: 'depositReduced', + marker: reportT('sourceReports.table.depositReducedInlineLabel'), + text: reportT('sourceReports.table.depositReducedFootnote'), + }); + } + // Build cover letter (if enabled) let coverLetter: ReportContentCoverLetter | null = null; if (includeCoverLetter) { From 4d936609e84c371634059ffb545ba598272d33a9 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 22:01:18 +0200 Subject: [PATCH 3/8] test(reports): strengthen #1965 legend assertion quality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace two weak negative marker assertions (not.toContain symbol) with positive equality checks against content.labels, add locale-aware text assertions proving real i18n resolution, add rendered-surface check for the split legend sentence (AC 3.1), and update the ReportContentEditor fixture from stale dagger/double-dagger symbols to current inline-label style. Note: getByText with a string fails when the marker contains NBSP — the lib normalizes element text (NBSP→space) but does NOT normalize the matcher string before ===. Use a regex (/^less\sdeposit:$/) which IS tested against the already-normalized text. Co-Authored-By: Claude qa-integration-tester --- .../story-1965-report-legend.md | 33 +++++++++++++++++++ .../reports/ReportContentEditor.test.tsx | 19 +++++++---- client/src/lib/reportPdf/realRender.test.ts | 14 ++++++-- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md b/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md index 390855128..1745a0005 100644 --- a/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md +++ b/.claude/agent-memory/qa-integration-tester/story-1965-report-legend.md @@ -9,6 +9,7 @@ Story #1965 reinstated legend sentences for split and depositReduced footnotes i `buildReportContent.ts` after they were removed in #1959. **Key changes (2026-08-03):** + - `buildReportContent.ts` now pushes `{id:'split', marker:..., text:...}` to `footnotes[]` when `splitInvoiceIds.size > 0`, and `{id:'depositReduced'...}` when `depositReducedInvoiceIds.size > 0`. @@ -22,6 +23,7 @@ correct fix. **goneFootnotes extension pattern:** To assert a text was REMOVED from goneFootnotes AND add a positive check it IS now present: + 1. Remove the string from `goneFootnotes` array 2. Add `depositFootnoteText: ''` to the `expected` object for each locale 3. Right after the goneFootnotes inner-for loop, add: @@ -33,3 +35,34 @@ To assert a text was REMOVED from goneFootnotes AND add a positive check it IS n **Coverage result:** `buildReportContent.ts` — 100% statements, 97.5% branches (uncovered: `if (options?.includeCoverLetter ?? false)` null-coalescence truthy path, not reachable from the new footnote tests). + +**Assertion strengthening round (2026-08-03):** +Three improvements landed in the same PR to tighten #1965 test assertions: + +1. **Weak-to-strong marker assertions:** Replaced `not.toContain('†')` / `not.toContain('‡')` + with `toBe(content.labels.splitNote)` / `toBe(content.labels.depositReducedNote)` — proves the + marker is the actual inline label, not merely "not a symbol". + +2. **Text assertions:** Added `toBe(expected.splitFootnoteText)` / `toBe(expected.depositFootnoteText)` + using the locale-specific values, proving real i18n bundle resolution (not key echo). + +3. **Rendered-surface AC 3.1:** Added `splitFootnoteText` to the locale expected objects and added + `expect(allStrings.some((s) => s.includes(splitFootnoteText))).toBe(true)` after the existing + deposit-reduced rendered-surface check. + +**Fix 3 gotcha — NBSP + testing-library string matcher:** +`ReportContentEditor.test.tsx` stale fixture (`†`/`‡`) updated to use current marker format. +The marker `'less deposit'` (which is `'less deposit'` from locale — NBSP encoded as `\xc2\xa0`) +caused `getByText('less deposit:')` to FAIL because testing-library's `matches()` normalizes the +**element text** (NBSP→space) but compares against the **raw un-normalized matcher string**: +``` +normalizedText === String(matcher) +// 'less deposit:' === 'less deposit:' → FALSE +``` +Fix: use a regex, which IS tested against the already-normalized text: +```ts +expect(screen.getByText(/^less\sdeposit:$/)).toBeInTheDocument(); +``` +The `\s` matches the plain space that NBSP normalizes to. This is a general pattern: whenever a +marker or label contains NBSP (from a locale file), use a regex for the `getByText` assertion, not +a plain string — they will never `===`-match the normalized element text. diff --git a/client/src/components/reports/ReportContentEditor.test.tsx b/client/src/components/reports/ReportContentEditor.test.tsx index 59b6f1b15..24b9f4ec9 100644 --- a/client/src/components/reports/ReportContentEditor.test.tsx +++ b/client/src/components/reports/ReportContentEditor.test.tsx @@ -938,20 +938,27 @@ describe('ReportContentEditor — summary rows and footnotes', () => { it('renders each footnote with its unnumbered/shared marker and text, read-only', () => { const content = makeContent({ footnotes: [ - { id: 'split', marker: '†', text: 'Amount shown reflects only the portion allocated.' }, { - id: 'deposit-reduced', - marker: '‡', + id: 'split', + marker: 'partial', + text: 'Amount shown reflects only the portion allocated to this source.', + }, + { + id: 'depositReduced', + marker: 'less deposit', text: 'This position reflects deposits claimed separately.', }, ], }); renderEditor({ content }); - expect(screen.getByText('†:')).toBeInTheDocument(); + expect(screen.getByText('partial:')).toBeInTheDocument(); expect( - screen.getByText(/Amount shown reflects only the portion allocated\./), + screen.getByText(/Amount shown reflects only the portion allocated to this source\./), ).toBeInTheDocument(); - expect(screen.getByText('‡:')).toBeInTheDocument(); + // getByText with a string fails when marker has NBSP: the lib normalizes element text + // (NBSP→space) but does NOT normalize the matcher, so ==='less deposit:' always mismatches. + // Regex is tested against the already-normalized text, so \s matches the collapsed space. + expect(screen.getByText(/^less\sdeposit:$/)).toBeInTheDocument(); }); it('renders no footnotes block when footnotes is empty', () => { diff --git a/client/src/lib/reportPdf/realRender.test.ts b/client/src/lib/reportPdf/realRender.test.ts index df779fafe..613ec0409 100644 --- a/client/src/lib/reportPdf/realRender.test.ts +++ b/client/src/lib/reportPdf/realRender.test.ts @@ -978,6 +978,7 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { // The footnote WORDINGS that must no longer appear anywhere in the tree. goneFootnotes: ['This is a deposit'], depositFootnoteText: 'This position reflects deposits claimed separately.', + splitFootnoteText: 'Amount shown reflects only the portion allocated to this source.', }, ], [ @@ -992,6 +993,8 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { goneFootnotes: ['Dies ist eine Abschlagszahlung'], depositFootnoteText: 'Diese Position berücksichtigt separat eingereichte Abschlagszahlungen.', + splitFootnoteText: + 'Der angezeigte Betrag umfasst nur den dieser Quelle zugeordneten Anteil.', }, ], ] as const) { @@ -1012,8 +1015,12 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { expect(content.footnotes).toHaveLength(2); expect(content.footnotes[0]!.id).toBe('split'); expect(content.footnotes[1]!.id).toBe('depositReduced'); - expect(content.footnotes[0]!.marker).not.toContain('†'); - expect(content.footnotes[1]!.marker).not.toContain('‡'); + // Markers match the inline labels (AC 2.3) — shared token between legend and row cell + expect(content.footnotes[0]!.marker).toBe(content.labels.splitNote); + expect(content.footnotes[1]!.marker).toBe(content.labels.depositReducedNote); + // Text values resolve from real i18n bundles, not echoed keys (real render, not mock t) + expect(content.footnotes[0]!.text).toBe(expected.splitFootnoteText); + expect(content.footnotes[1]!.text).toBe(expected.depositFootnoteText); const pdfContent = buildOverviewContent(content, new Map(), t); const tableItem = pdfContent.find( @@ -1054,6 +1061,9 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { // #1965: the deposit-reduced legend sentence IS present in the rendered content. const depositFootnoteText = expected.depositFootnoteText; expect(allStrings.some((s) => s.includes(depositFootnoteText))).toBe(true); + // #1965 AC 3.1: split legend sentence also present in the rendered content tree + const splitFootnoteText = expected.splitFootnoteText; + expect(allStrings.some((s) => s.includes(splitFootnoteText))).toBe(true); } }); From 20dd6f4e631fea96aa26b24b9dd2f6f0946af8c8 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 22:03:41 +0200 Subject: [PATCH 4/8] fix(reports): address review findings for #1965 legend reinstatement - Fix JSX whitespace in ReportContentEditor footnote list (preview/PDF parity) - Invert Scenario 18 E2E assertions: expect 1 legend entry, not 0 - Update POM directive and spec header for new footnotes behaviour Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude e2e-test-engineer --- .../issue-1959-inline-meta-and-labels.md | 25 ++++++++--- .../reports/ReportContentEditor.tsx | 3 +- e2e/pages/ReportWizardPage.ts | 13 +++--- .../reportWizardEditableContent.spec.ts | 43 +++++++++++-------- 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md b/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md index 5375285b2..ffc5485fc 100644 --- a/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md +++ b/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md @@ -1,6 +1,6 @@ --- name: issue-1959-inline-meta-and-labels -description: PR #1959 reversed two earlier report-table designs (†/‡ shared footnotes from #1923, distinct area sub-line) into inline labels + one combined meta line; which E2E locators/scenarios had to be rewritten and how each new assertion was made non-vacuous. +description: PR #1959 reversed two earlier report-table designs (†/‡ shared footnotes from #1923, distinct area sub-line) into inline labels + one combined meta line; which E2E locators/scenarios had to be rewritten and how each new assertion was made non-vacuous. Issue #1965 then reinstated legend footnotes for split/depositReduced rows. metadata: type: project --- @@ -9,11 +9,22 @@ PR #1959 ("improve report PDF UX") deliberately **superseded** two designs earli asked for, in `ReportContentEditor.tsx` / `buildReportContent.ts`: 1. `†`/`‡` markers + the shared footnote list (Story #1923 AC1) → grey inline `` in the **Allocated Amount cell**: `(partial)` / `(less deposit)` +class*="inlineNote">` in the **Allocated Amount cell**: `(partial)` / `(less deposit)` (de `(Teilbetrag)` / `(abzgl. Abschlag)`). `ReportContentRow.allocatedMarkers` → `isSplit` / - `isDepositReduced` booleans. `buildReportContent` now pushes **zero** footnotes, so - `.footnotes` has no producer at all — `footnotesBlock`/`footnoteItems` survive in the POM as + `isDepositReduced` booleans. `buildReportContent` pushed **zero** footnotes after #1959, so + `.footnotes` had no producer — `footnotesBlock`/`footnoteItems` survived in the POM as **negative-only** guards. + + **Issue #1965 update (fix/report-pdf-ux-improvements branch):** `buildReportContent.ts` now + pushes ONE deduplicated legend entry per active flag: `splitInvoiceIds.size > 0` → one `'split'` + footnote ("Amount shown reflects only the portion allocated to this source."), + `depositReducedInvoiceIds.size > 0` → one `'depositReduced'` footnote. `footnotesBlock` / + `footnoteItems` are NO LONGER negative-only guards. Scenarios with split or deposit-reduced rows + must assert a **positive** count; constituted-deposit-only rows (Scenario 17) still assert + `toHaveCount(0)` because neither set is non-empty for them. Scenario 18 (two split invoices) + asserts `footnotesBlock` count=1, `footnoteItems` count=1, and the legend sentence IS present in + `main`'s text content. + 2. `.usageAreaText` sub-line + the separate editable `Attachments Note` column → ONE read-only `.usageMetaText` line inside the Usage cell: `[areaText, attachmentsNote].join(' · ')` (U+00B7 middle dot, spaces on both sides). The `attachmentsNote` `EditableField` is gone @@ -27,9 +38,9 @@ the PR body the spec, so the tests were rewritten, not the code. `mobileUsageAreaText`→`mobileUsageMetaText`, plus new `inlineNote()`/`mobileInlineNote()`; `attachmentsNoteField()` deleted. Rewritten scenarios: editableContent 2, 17, 18, 20 and aiGeneration 8. Every "old design is gone" negative is paired with a positive so it cannot pass -against a mis-seeded page (e.g. Scenario 18 asserts `(partial)` present *and* `†`/`‡` absent -*and* the long-form footnote sentence absent from `main`; Scenario 20 asserts the attachments -note text IS rendered *and* the row has one textbox). +against a mis-seeded page (e.g. Scenario 18 asserts `(partial)` present _and_ `†`/`‡` absent +_and_ the long-form footnote sentence absent from `main`; Scenario 20 asserts the attachments +note text IS rendered _and_ the row has one textbox). Facts worth reusing: diff --git a/client/src/components/reports/ReportContentEditor.tsx b/client/src/components/reports/ReportContentEditor.tsx index 5e3505d48..cde6366f6 100644 --- a/client/src/components/reports/ReportContentEditor.tsx +++ b/client/src/components/reports/ReportContentEditor.tsx @@ -458,8 +458,7 @@ export function ReportContentEditor({
    {content.footnotes.map((note) => (
  • - {note.marker}: - {note.text} + {note.marker}: {note.text}
    • ))}
diff --git a/e2e/pages/ReportWizardPage.ts b/e2e/pages/ReportWizardPage.ts index 1c013003c..19cd220cf 100644 --- a/e2e/pages/ReportWizardPage.ts +++ b/e2e/pages/ReportWizardPage.ts @@ -469,11 +469,14 @@ export class ReportWizardPage { // avoids a substring collision). readonly summaryTable: Locator; readonly summaryTableRows: Locator; - // The footnotes block (`.footnotes` / its `
  • ` entries). As of Issue #1959 NOTHING populates - // `content.footnotes` any more (the `†`/`‡` split + deposit-reduced entries became inline - // labels — see `inlineNote()`), so the block is never rendered. Kept purely as a NEGATIVE - // guard: a scenario asserting `toHaveCount(0)` / `not.toBeVisible()` here is asserting that - // the superseded footnote mechanism has not come back. Never assert a positive count on these. + // The footnotes block (`.footnotes` / its `
  • ` entries). Issue #1959 replaced the `†`/`‡` + // split + deposit-reduced glyphs with inline labels (see `inlineNote()`), but Issue #1965 + // restored legend population: `buildReportContent.ts` now pushes ONE deduplicated sentence per + // active flag (`isSplit` → "Amount shown reflects only the portion allocated to this source.", + // `isDepositReduced` → the corresponding deposit-reduced sentence) whenever any row in the + // report carries that flag. Scenarios with split or deposit-reduced rows must assert a positive + // count; scenarios with neither (e.g., constituted-deposit-only rows — Scenario 17) correctly + // assert `toHaveCount(0)`. readonly footnotesBlock: Locator; readonly footnoteItems: Locator; diff --git a/e2e/tests/budget/reportWizardEditableContent.spec.ts b/e2e/tests/budget/reportWizardEditableContent.spec.ts index 1fb615534..508d8d2a3 100644 --- a/e2e/tests/budget/reportWizardEditableContent.spec.ts +++ b/e2e/tests/budget/reportWizardEditableContent.spec.ts @@ -75,8 +75,9 @@ * attachments note into ONE read-only grey meta line in the Usage cell. See `ReportWizardPage.ts`'s * class docstring for the full locator reference (`sourceInfoBlock`, `depositBadge`/ * `mobileDepositBadge`, `inlineNote`/`mobileInlineNote`, `usageMetaText`/`mobileUsageMetaText`, - * `summaryTable`/`summaryTableRows`, and `footnotesBlock`/`footnoteItems` — retained as - * negative-only guards now that nothing populates `content.footnotes`). + * `summaryTable`/`summaryTableRows`, and `footnotesBlock`/`footnoteItems` — zero for + * constituted-deposit-only scenarios (Scenario 17), one deduplicated entry for split/ + * deposit-reduced scenarios (Issue #1965)). * - Scenario 16: A `claim` report omits the source-info metadata block entirely (AC3.1) — the * counterpart to Scenario 1's `budget-overview` regression guard (AC3.3). * - Scenario 17: A constituted-deposit row (the row's allocation is made up entirely by a @@ -84,9 +85,10 @@ * desktop, tablet, AND mobile, carries NO inline `(partial)`/`(less deposit)` note (nor either * legacy `†`/`‡` glyph), and there is no footnotes block at all (AC2.1, AC2.2). * - Scenario 18: Every split invoice carries its OWN inline `(partial)` label in its Allocated - * Amount cell (Issue #1959, superseding AC1.1-AC1.2's shared unnumbered `†` marker), and the - * footnote list — including the long-form "Amount shown reflects only the portion allocated to - * this source." sentence — is gone from the page entirely. + * Amount cell (Issue #1959, superseding AC1.1-AC1.2's shared unnumbered `†` marker). The + * footnote list now contains exactly ONE deduplicated legend sentence ("Amount shown reflects + * only the portion allocated to this source.") pushed by `buildReportContent.ts` because + * `splitInvoiceIds.size > 0` (Issue #1965). * - Scenario 19: Invoices spanning two or more statuses still produce exactly one summary row * (`Total`) — no per-status subtotal rows (AC4.1-AC4.2). * - Scenario 20: A budget line linked to an item with an assigned area shows the item's leaf @@ -1620,11 +1622,13 @@ test.describe( expect(rowText).not.toContain('‡'); } - // No footnote entry at all — no split (B has zero budget lines here), no - // deposit-reduced (the deposit IS tagged to B, not "reduced"), the removed - // `depositConstitutedFootnote` key ("This is a deposit.") never had a footnote to begin - // with even before Story #1923, and as of Issue #1959 nothing populates - // `content.footnotes` at all any more. + // No footnote entry: this is a constituted-deposit row — the allocation is made up + // entirely by a deposit tagged to source B, so `isSplit` is false (only source B has + // budget lines; the invoice is not split across sources), and `isDepositReduced` is also + // false (the deposit constitutes the row, it does not reduce a gross amount). + // `buildReportContent.ts` only pushes legend entries when `splitInvoiceIds.size > 0` or + // `depositReducedInvoiceIds.size > 0` — neither condition holds here, so + // `content.footnotes` stays empty and the block is absent from the DOM (Issue #1965). await expect(wizard.footnotesBlock).toHaveCount(0); } finally { if (workItemId) await deleteWorkItemViaApi(page, workItemId); @@ -1710,22 +1714,25 @@ test.describe('Report wizard editable content — inline split label (Scenario 1 await expect(row1).toContainText('€100.00 (partial)'); await expect(row2).toContainText('€150.00 (partial)'); - // Negatives, each paired with the positives above: neither footnote glyph survives - // anywhere in the row (numbered or not), and the footnote list has no producer left — the - // block is absent from the DOM entirely rather than merely empty. + // Negatives, each paired with the positives above: neither legacy footnote glyph survives + // anywhere in the row (numbered or not). const row1Text = (await row1.textContent()) ?? ''; const row2Text = (await row2.textContent()) ?? ''; expect(row1Text).not.toContain('†'); expect(row2Text).not.toContain('†'); expect(row1Text).not.toContain('‡'); expect(row2Text).not.toContain('‡'); - await expect(wizard.footnotesBlock).toHaveCount(0); - await expect(wizard.footnoteItems).toHaveCount(0); - // The long-form footnote sentence the marker used to point at is gone from the page too - // (it now only exists as the short inline label asserted above). + // Issue #1965: `buildReportContent.ts` now pushes ONE deduplicated legend entry to + // `content.footnotes` whenever `splitInvoiceIds.size > 0`. Both invoices in this fixture + // are split, so the block must be present with exactly 1 item — deduped even though two + // rows triggered it. + await expect(wizard.footnotesBlock).toHaveCount(1); + await expect(wizard.footnoteItems).toHaveCount(1); + + // The long-form legend sentence must be present in the footnote list. const pageText = (await page.locator('main').textContent()) ?? ''; - expect(pageText).not.toContain( + expect(pageText).toContain( 'Amount shown reflects only the portion allocated to this source.', ); expect(pageText).toContain('(partial)'); From 5f1da1219a85924ba81efb5c81e6a377e38b00a0 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 22:04:50 +0200 Subject: [PATCH 5/8] docs: update wiki submodule to ADR-034 legend correction (#1965) Co-Authored-By: Claude product-architect --- wiki | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wiki b/wiki index df8a462fc..f9cd21c8d 160000 --- a/wiki +++ b/wiki @@ -1 +1 @@ -Subproject commit df8a462fcfef88610781a49258fdccf2f92d45ee +Subproject commit f9cd21c8db3f98b1315aeadaa0287ce5ae241cfc From d360e1d8dea2c7a5e773f697b028bc0aae7a1ff5 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 22:05:26 +0200 Subject: [PATCH 6/8] docs(memory): record #1965 document-level legend model and ADR-034 correction Co-Authored-By: Claude product-architect --- .../agent-memory/product-architect/MEMORY.md | 4 +-- .../product-architect/client-pdf-pipeline.md | 35 +++++++++++++++---- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index 373f93876..79de1d176 100644 --- a/.claude/agent-memory/product-architect/MEMORY.md +++ b/.claude/agent-memory/product-architect/MEMORY.md @@ -2,11 +2,11 @@ ## Topic Files -- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), AC reversal by a polish issue (#1959) +- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959) - [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, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule + **owed ADR-034 corrections** (#1959) +- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) + **owed ADR-034 corrections** (#1959) - [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) diff --git a/.claude/agent-memory/product-architect/client-pdf-pipeline.md b/.claude/agent-memory/product-architect/client-pdf-pipeline.md index 746bccc37..06b266554 100644 --- a/.claude/agent-memory/product-architect/client-pdf-pipeline.md +++ b/.claude/agent-memory/product-architect/client-pdf-pipeline.md @@ -81,14 +81,14 @@ change to `overviewPdf.ts` widths, `TABLE_LAYOUT`, or `pageMargins` must be vali `/CreationDate` + `/ID`); a unit test asserting `TABLE_LAYOUT.dontBreakRows === true` passes and proves nothing. 2. **Declared widths are CONTENT widths.** pdfmake subtracts `_offsets.total` from the available - width *before* distributing them. `offsetsTotal = cols * (paddingLeft + paddingRight + - vLineWidth) + vLineWidth`. With `TABLE_LAYOUT`'s `8/8/0.5` that is **116.0pt for 7 columns, + width _before_ distributing them. `offsetsTotal = cols * (paddingLeft + paddingRight + +vLineWidth) + vLineWidth`. With `TABLE_LAYOUT`'s `8/8/0.5` that is **116.0pt for 7 columns, 99.5pt for 6** out of the 515.28pt A4 printable width. Budget columns against `515.28 - offsetsTotal(cols)`, not 515.28. Getting this wrong made a comment claim Usage got 185.28pt when it actually got **69.28pt**. 3. **A `'*'` column never shrinks below its longest unbreakable word.** `columnCalculator.js:66-75` — when `minW >= availableWidth` the star is set to `starMaxMin` and - *the table overflows the page*. So no static assertion on the `widths` array can prove "no + _the table overflows the page_. So no static assertion on the `widths` array can prove "no horizontal overflow": German compounds (`Wärmedämmverbundsystem` ~128pt @10pt Roboto) push a 69.28pt star to 128pt and the table to 574pt on a 515.28pt page. 4. **`dontBreakRows` + a row taller than the printable height = silent data loss.** pdfmake does @@ -234,13 +234,13 @@ document): page count must be **monotonic** in content size, and **channel-indep costs the same paper whichever field carries it. **The generic-assertion / hand-enumerated-input asymmetry** (the reason it recurred, now the top open item): -the per-row budget assertion counts characters generically over all runs, so a new channel *would* be +the per-row budget assertion counts characters generically over all runs, so a new channel _would_ be counted — but the test inputs are hand-listed (`usageText`/`areaText`/`attachmentsNote`), so a new `ReportContentRow` string field defaults empty and every assertion passes **vacuously**. Fix is key-driven saturation (`Object.entries(row)` -> saturate every string field), not another hand-written case. Verified empirically during review: losslessness + per-row budget hold over 3k fuzzed inputs (0 failures); a hypothetical **second** grey/meta segment trips the existing `splitUsageCell` "at most one grey run per row" -throw in ~49% of inputs, so *that* channel class is already guarded. +throw in ~49% of inputs, so _that_ channel class is already guarded. **Geometry constraint (blocks a feature):** `USAGE_WIDTH_7COL/_6COL` derive from `usableColumnWidth(n)`, and `MAX_SAFE_USAGE_CHUNK_CHARS = 650` was **measured against the 7-column shape**. So making column visibility @@ -251,11 +251,34 @@ through" is a re-measurement story, not a UI change. #1959's toggles are preview New to the packer — `splitIntoPageSafeChunks` fails loudly instead (`RangeError`). Unreachable while the budget is a constant; matters if it ever becomes computed. +### Legend is document-level, not per-row (#1965) + +DONE 2026-08-03: ADR-034 records this (wiki master `03ed804`, addendum "the legend is document-level and +deduplicated" + a Deviation Log row + a reworded B4 rule). The old B4 wording ("every footnote is referenced +from the row that owns it") described the pre-#1965 inline-symbol design (`†`/`‡`) and was **wrong** for the +current model. + +Two structurally different note kinds now share the legend block but **not** a numbering scheme: + +- **`*N` skipped-document notes** — numbered, row-owned, one per skipped document, built in `overviewPdf.ts` + at generation time (never in `ReportContent`). B4's "referenced from the owning row" rule applies here only. +- **`content.footnotes[]` legend entries** — **at most one per flag type for the whole document** + (currently 2: `split`, `depositReduced`). `buildReportContent.ts` accumulates `splitInvoiceIds` / + `depositReducedInvoiceIds` as `Set` and pushes gated on `set.size > 0`, so cardinality is + independent of how many rows carry the flag. `marker` is the repeated human-readable inline label + (`partial` / `less deposit`, report-language) that the Allocated Amount cell prints, **not** an identifier + — `id` is the machine key. Row↔legend link is **by repetition of the label**, not by stored reference; + rows carry only `isSplit`/`isDepositReduced` booleans. Storing a footnote index on a row would recreate + B4's second numbering namespace. + +Regression to guard when adding a flag type: emitting one entry per flagged row. Assert +`footnotes.length === N` (never `>= 1`) on a fixture where several rows share a flag. + ### ADR-034 debt (owed, NOT yet written — carry this forward) 1. Add the `dontBreakRows` lesson + the "bound the rendered cell, not a field" rule + both detection recipes. 2. **Minimum-bar rule #1 is wrong**: `table._minWidth <= 515.28` fails on correct code (`_minWidth` is the - widest unbreakable *word*, not the laid-out width). Correct check: `max(horizontalRatio) <= 1`. + widest unbreakable _word_, not the laid-out width). Correct check: `max(horizontalRatio) <= 1`. B2's narrative is fine; the generalized rule was mis-transcribed. 3. Module table drifted twice: add `pageGeometry.ts` (#1939) and `index.ts`; drop "PDF-local formatters" from `shared.ts` (deleted in review round 2) and move "table layout constants" to `pageGeometry.ts`. From a9c1ebc9ad373b7df59a474c32794c4878b83009 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 22:21:41 +0200 Subject: [PATCH 7/8] docs(memory): record ADR-034 legend model and the half-done multi-site fix pattern Discharge the owed-ADR-034 note now that PR #1979 landed the B4 rewording, legend addendum, and Deviation Log. Record the re-review pattern: an enumerated multi-site doc fix comes back covering only the sites adjacent to the changed code, and a test title can state the inverse of its body. Co-Authored-By: Claude product-architect --- .../agent-memory/product-architect/MEMORY.md | 2 +- .../product-architect/client-pdf-pipeline.md | 26 ++++++++++++ .../product-architect/recurring-patterns.md | 42 ++++++++++++++++++- 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index 79de1d176..96cac9dfa 100644 --- a/.claude/agent-memory/product-architect/MEMORY.md +++ b/.claude/agent-memory/product-architect/MEMORY.md @@ -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, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) + **owed ADR-034 corrections** (#1959) +- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) — ADR-034 B4 rule + legend addendum + Deviation Log landed in PR #1979, discharging the #1959 debt - [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) diff --git a/.claude/agent-memory/product-architect/client-pdf-pipeline.md b/.claude/agent-memory/product-architect/client-pdf-pipeline.md index 06b266554..c44827a07 100644 --- a/.claude/agent-memory/product-architect/client-pdf-pipeline.md +++ b/.claude/agent-memory/product-architect/client-pdf-pipeline.md @@ -284,3 +284,29 @@ Regression to guard when adding a flag type: emitting one entry per flagged row. `shared.ts` (deleted in review round 2) and move "table layout constants" to `pageGeometry.ts`. 4. Override-key list (line 148): drop `attachmentsNote` — unreachable since #1959. 5. Record the fixed 6-or-7 column-count constraint above. + +## ADR-034 legend model, corrected in PR #1979 (wiki `03ed804`) + +The ADR-034 debt owed since #1959 is now paid. Two structurally different note kinds share the block below +the overview table and must never share a numbering scheme: + +| Kind | Marker | Cardinality | Built by | +| --- | --- | --- | --- | +| Skipped-document note | `*N`, numbered, referenced by the owning row | one per skipped document | `overviewPdf.ts` at generation time (not in `ReportContent`) | +| Legend entry (`content.footnotes[]`) | repeated inline word label — `partial`, `less deposit` | **at most one per flag type per document** | `buildReportContent.ts` | + +B4's old generalized rule ("every footnote is referenced from the row that owns it") applied only to the +numbered kind and was reworded. Invariants now recorded in the ADR's legend addendum: + +- `footnotes[].marker` is `sourceReports.table.{split,depositReduced}InlineLabel` — the *same* keys as + `labels.{splitNote,depositReducedNote}` and as the inline label the row cell prints. Row↔legend joins by + **repetition of that literal**, not by id/index/number. NBSP in `less deposit` / `abzgl. Abschlag` is + load-bearing; `expect(footnotes[0].marker).toBe(content.labels.splitNote)` is the assertion that pins it. +- Gated on `splitInvoiceIds.size > 0` / `depositReducedInvoiceIds.size > 0` (`Set` accumulated in the + `includedInvoiceIds`-filtered row loop), so `footnotes.length` is bounded by flag count (2), never row count. +- Adding a flag type = new `Set` + `size > 0` push in `buildReportContent.ts`, new boolean on + `ReportContentRow`, new inline label in `overviewPdf.ts`. Assert exact `footnotes.length` (not `>= 1`) on a + fixture where several rows share a flag. +- Preview/export parity trap: once markers became *words*, `ReportContentEditor`'s + `{marker}:{text}` ran them together while the PDF used `${marker}: ${text}`. Fixed in #1979 — + any change to either surface must keep the separator identical. diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index 3f2327fbe..b6b21c190 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -406,6 +406,23 @@ Rule: **delete rather than comment.** A write path with no reader and no produce round-3/round-4 confusion started — the code said one thing and the comment said another. When triaging a "keep it as a capability?" question, check for a _producer_ first: no producer => dead, remove it. +## Reinstating a removed producer breaks the negative guards left behind (#1965 / PR #1979) + +The exact inverse of the pattern above, and it bites one story later. #1959 removed the `content.footnotes` +producer and left **negative-only guards plus a prose directive**: `ReportWizardPage.ts` said "NOTHING +populates `content.footnotes` … Never assert a positive count on these", and Scenario 18 asserted +`footnotesBlock/footnoteItems` count 0 + the sentence absent from `main`. #1965 restored the producer with a +3-line push in `buildReportContent.ts` — and silently turned a green 3-viewport E2E scenario red. + +**Review rule for any "reinstate / re-enable X" PR:** `grep` the whole repo (especially `e2e/pages/*` and +spec-file header docstrings) for assertions and _directives_ that pin X's absence. A PR that adds a producer +without inverting those is incomplete, and because `E2E Gates` is `main`-only it merges green into `beta` and +surfaces only at promotion (see [[merge-gate-vs-done-gate]] / the beta-merges-past-red-E2E trap). + +Corollary: a stale POM docstring is worse than a stale code comment — it is an instruction later agents obey. +Inverting the E2E assertion usually also discharges the story's "count occurrences in the rendered DOM" AC, so +it is the same edit, not extra work. + ## Staleness tokens: the `finally` block is the hole (#1946 / PR #1977) A monotonic-token guard (`if (ref.current !== token) return;` in `.then`/`.catch`) does **not** protect a @@ -453,7 +470,7 @@ apart again. Playwright POMs address dialogs by accessible name (`page.getByRole('dialog', { name: 'Discard your edits?' })`, `e2e/pages/ReportWizardPage.ts`), so making a title conditional silently narrows that locator to one branch. -In PR #1977 nothing broke — the only E2E usage opens the modal *after* generation resolved, so the old title +In PR #1977 nothing broke — the only E2E usage opens the modal _after_ generation resolved, so the old title still renders — but the POM docstring now documents the title as unconditional, and the next test that opens the modal mid-generation will fail to find the dialog. **Whenever a PR conditionalizes any modal/heading string, grep `e2e/pages/` for the literal** and flag the locator + docstring as an e2e-test-engineer follow-up. Same @@ -471,3 +488,26 @@ requirements reversal authored as a polish issue is the signature.** Cheap fix: old issue + PO ratification on the new one. Check this whenever a PR deletes user-visible report/document content — and check whether the replacement text preserves _meaning_ (`(abzgl. Abschlag)` lost the footnote's "claimed separately", which is compliance-relevant in a bank-facing document). + +## Enumerated multi-site doc fixes come back half-done (PR #1979 r2) + +When a review finding names N sites for the same stale claim, expect the fix commit to update the *nearest* +ones and miss the rest. #1979's HIGH 2 named four sites for "nothing populates `content.footnotes`"; the fix +updated the field-declaration comment and the spec header (both adjacent to the changed assertions) and left +the two class-docstring paragraphs — which contained the strongest form ("they can never be populated by the +current code path, and any test asserting a footnote `
  • ` is asserting a superseded design"). + +Two habits that follow: + +- **Re-grep the literal on re-review**, never trust the fix commit's diff to cover the enumeration. One + `grep -n -i footnote e2e/pages/ReportWizardPage.ts` found both misses instantly. +- **Check the test *name*, not just the body.** #1979 inverted Scenario 18's assertions to `toHaveCount(1)` + but left the Playwright title reading "and no footnote list anywhere on the page". A title that states the + inverse of its body is worse than a stale comment: it renders that way in every CI report and is the first + artifact a future reader uses to conclude the *body* drifted. Same for the `// Scenario NN:` block header. + +Why this is worth blocking on (I did, r2): the POM class docstring is the contract the spec header points at +("See `ReportWizardPage.ts`'s class docstring for the full locator reference"), so a directive there plus a +lying test title is a complete instruction set for deleting the coverage the PR exists to add — and with +`E2E Gates` main-only, that deletion lands on `beta` silently. It is the same mechanism that produced #1965: +#1959 removed a producer and left comments asserting the removal was permanent. From 6053ac4fb45984cdc7a5efa463539bb2968e0d77 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 22:31:06 +0200 Subject: [PATCH 8/8] =?UTF-8?q?test(reports):=20final=20review=20fixes=20f?= =?UTF-8?q?or=20#1965=20=E2=80=94=20parity=20guard=20and=20stale=20POM=20c?= =?UTF-8?q?omments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add toHaveTextContent parity guard on footnote
  • to detect missing space - Update ReportWizardPage POM: remove stale "never populate" directives, document that footnotesBlock/footnoteItems are now populated for split/depositReduced rows - Correct Scenario 18 test title and section banner to reflect one legend entry Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude e2e-test-engineer --- .../reports/ReportContentEditor.test.tsx | 10 +++++++++ e2e/pages/ReportWizardPage.ts | 21 ++++++++++++------- .../reportWizardEditableContent.spec.ts | 6 +++--- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/client/src/components/reports/ReportContentEditor.test.tsx b/client/src/components/reports/ReportContentEditor.test.tsx index 24b9f4ec9..6faf7d08b 100644 --- a/client/src/components/reports/ReportContentEditor.test.tsx +++ b/client/src/components/reports/ReportContentEditor.test.tsx @@ -959,6 +959,16 @@ describe('ReportContentEditor — summary rows and footnotes', () => { // (NBSP→space) but does NOT normalize the matcher, so ==='less deposit:' always mismatches. // Regex is tested against the already-normalized text, so \s matches the collapsed space. expect(screen.getByText(/^less\sdeposit:$/)).toBeInTheDocument(); + // Whitespace-parity guard (#1965): the
  • 's combined text must be "partial: Amount shown…" — + // marker, a single space, then the note text. jest-dom's toHaveTextContent normalises whitespace + // (including NBSP→space) in the element's textContent before comparing, so this catches a + // missing space (or extra space) between the and the text node without being fragile to + // NBSP, while the independent getByText checks above cannot detect inter-node spacing defects. + // Locate via the already-proven marker and walk up to the enclosing
  • . + const splitLi = screen.getByText('partial:').closest('li') as HTMLElement; + expect(splitLi).toHaveTextContent( + 'partial: Amount shown reflects only the portion allocated to this source.', + ); }); it('renders no footnotes block when footnotes is empty', () => { diff --git a/e2e/pages/ReportWizardPage.ts b/e2e/pages/ReportWizardPage.ts index 19cd220cf..04c73601f 100644 --- a/e2e/pages/ReportWizardPage.ts +++ b/e2e/pages/ReportWizardPage.ts @@ -205,8 +205,8 @@ * `budget-overview`/`proof-of-funds` reports are unaffected (still render it). * - The `†` (split) / `‡` (deposit-reduced) markers this story made shared/unnumbered were * REPLACED WHOLESALE by inline labels in Issue #1959 — see that paragraph below. Neither glyph - * appears anywhere in the UI or the PDF any more, and `footnotesBlock`/`footnoteItems` - * (declared above) consequently have no producer left. + * appears anywhere in the UI or the PDF any more. `footnotesBlock`/`footnoteItems` had no + * producer at this point, but see Issue #1965 below — that changed. * - A constituted-deposit row (the allocation is made up entirely by a deposit tagged to the * reported source) carries NO marker/label at all — instead an inline `Badge` (`depositBadge`/ * `mobileDepositBadge` below) reading "Deposit"/"Abschlagszahlung". There is correspondingly @@ -220,11 +220,18 @@ * `mobileUsageMetaText()` below (Issue #1959). * * Issue #1959: report PDF/UI polish — inline meta, inline split/deposit labels, column toggles. - * - `†`/`‡` markers and the footnote LIST are GONE. `buildReportContent.ts` no longer pushes any - * `ReportContentFootnote` at all (`ReportContentRow.allocatedMarkers` was replaced by - * `isSplit`/`isDepositReduced` booleans), so `footnotesBlock`/`footnoteItems` are retained - * below purely as negative guards — they can never be populated by the current code path, and - * any test asserting a marker glyph or footnote `
  • ` is asserting a superseded design. + * - `†`/`‡` markers and the footnote LIST are GONE at this point. `buildReportContent.ts` no + * longer pushes any `ReportContentFootnote` (`ReportContentRow.allocatedMarkers` was replaced by + * `isSplit`/`isDepositReduced` booleans). `footnotesBlock`/`footnoteItems` were retained as + * negative guards only — until Issue #1965 reinstated the legend (see below). + * + * Issue #1965: report PDF legend — `buildReportContent.ts` now pushes legend entries for rows + * where `isSplit` or `isDepositReduced` is true, so `footnotesBlock`/`footnoteItems` ARE now + * populated in split/deposit-reduced scenarios. `footnotesBlock` contains the legend block (or + * is absent from the DOM entirely when no split/deposit-reduced rows appear); `footnoteItems` + * are the `
  • ` elements inside it, one per deduplicated flag type (so two split invoices + * produce exactly one `
  • ` entry, not two). Tests asserting `toHaveCount(1)` on these + * locators reflect the current design and are correct. * Instead, a split row appends a grey inline `` reading `(partial)` * (de: `(Teilbetrag)`, `sourceReports.table.splitInlineLabel`) INSIDE the Allocated Amount * cell, and a deposit-reduced row one reading `(less deposit)` (de: `(abzgl. Abschlag)`, diff --git a/e2e/tests/budget/reportWizardEditableContent.spec.ts b/e2e/tests/budget/reportWizardEditableContent.spec.ts index 508d8d2a3..0fcaddb96 100644 --- a/e2e/tests/budget/reportWizardEditableContent.spec.ts +++ b/e2e/tests/budget/reportWizardEditableContent.spec.ts @@ -1641,12 +1641,12 @@ test.describe( ); // ───────────────────────────────────────────────────────────────────────────── -// Scenario 18: Every split invoice carries its own inline "(partial)" label and there is no -// footnote list at all (Story #1923 AC1, superseded by Issue #1959) +// Scenario 18: Split invoices carry an inline "(partial)" label AND produce one deduplicated +// legend entry in the footnotes block (Issue #1965) // ───────────────────────────────────────────────────────────────────────────── test.describe('Report wizard editable content — inline split label (Scenario 18)', () => { - test('Two split invoices each show a grey inline "(partial)" note in their Allocated Amount cell, with no †/‡ marker and no footnote list anywhere on the page', async ({ + test('Two split invoices each show a grey inline "(partial)" note in their Allocated Amount cell, and produce exactly one deduplicated legend entry in the footnotes block', async ({ page, testPrefix, }) => {