Skip to content
Merged
Original file line number Diff line number Diff line change
@@ -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
---
Expand All @@ -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 `<span
class*="inlineNote">` 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
Expand All @@ -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:

Expand Down
4 changes: 2 additions & 2 deletions .claude/agent-memory/product-architect/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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) — 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)
Expand Down
61 changes: 55 additions & 6 deletions .claude/agent-memory/product-architect/client-pdf-pipeline.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -251,13 +251,62 @@ 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<string>` 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`.
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<string>` 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
`<span>{marker}:</span>{text}` ran them together while the PDF used `${marker}: ${text}`. Fixed in #1979 —
any change to either surface must keep the separator identical.
42 changes: 41 additions & 1 deletion .claude/agent-memory/product-architect/recurring-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 `<li>` 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.
Loading