feat(reports): rebuild the cover letter as a business letter (#1932)

[steilerDev]

Summary

The user's scope ruling shaped the whole story: plain text with line breaks, no WYSIWYG editor, no new client dependency.

• The body's line breaks already survived rendering (pdfmake treats \n as a required line end); what was actually missing was the letter itself. Blank-line preservation is now pinned by a real pdfmake-render assertion (line count, spacing, and an exact .text pin before/after render) instead of a content-tree string check.
• The ruling surfaced a genuine defect: the LLM prompt never constrained output format, so a generated letter could print literal **asterisks** and - hyphens in a PDF handed to a bank. Rule 4 of the report-content prompt now mandates plain prose (appended; Single "Enhance with AI" action and purpose-focused report content prompt #1931's pinned safety constraints are untouched). The wiki now also documents that this is a prompt-level instruction, not a validator-enforced guarantee.
• signature becomes a first-class editable field; an explicit signature edit is no longer silently overwritten when the sender is edited afterward (unit + real-render + E2E coverage, both key-insertion orders).
• The sender is now the logged-in person's display name plus the household address, not the household name.
• A real signature block — closing salutation, reserved signature space, signatory name — is emitted unconditionally: an empty pdfmake text node was measured to reserve the same line height as a non-empty one, so no NBSP workaround is needed. The Closing row (label + value, positioned between body and signature) now has dedicated unit coverage, verified by deleting its JSX and confirming exactly the expected tests fail.
• Letter hierarchy via varied margins, a right-aligned date, and a bold/larger subject style. The date/closing read-only captions render without a trailing colon (Report wizard preview: cover-letter date caption uses the UI language beside a report-language date #1925 AC5, now pinned on the editor side too).
• The German closing salutation drops the comma per DIN 5008 ("Mit freundlichen Grüßen") — English keeps it.
• The oversized reset-"X" in the shared EditableField is fixed: intrinsic width instead of full-container stretch, glyph sized off a font token instead of 100%, the 44×44 hit area kept, and a focus ring added where none existed before.

Folds in and supersedes #1925 (date-caption language mixing) — close it as a duplicate once this merges.

Fixes #1932

Test plan

• Unit tests pass (95%+ coverage) — AC 1.1–1.3/7.1/7.2 pinned via a real pdfmake render (line/blank-line count, uniform spacing, exact text preservation, literal markup survival), not content-tree string checks
• AC 2.6 (explicit signature survives a same-call sender edit) covered at the unit level (applyOverrides.test.ts), the real-render level (realRender.test.ts), and end-to-end (E2E Scenario 23), in both key-insertion orders
• Integration tests pass
• CI Quality Gates pass (typecheck, tests, build, audit)
• E2E: Scenarios 21–24 added to reportWizardEditableContent.spec.ts — signature + multi-paragraph body edit and PDF-preview round trip (desktop light, mobile dark), the AC 2.6 sender-after-signature regression guard, and the signature reset interaction

Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude backend-developer noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude translator noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer noreply@anthropic.com
Co-Authored-By: Claude ux-designer noreply@anthropic.com
Co-Authored-By: Claude product-owner noreply@anthropic.com
Co-Authored-By: Claude product-architect noreply@anthropic.com

[steilerDev]

[ux-designer]

Review of PR #1951 against my visual spec (issue #1932, comment 5160362068)

Verified by rendering the real, unmocked pdfmake pipeline (generateReportPdf) to actual PDF bytes via a throwaway scratch Jest test, then rasterizing with pdftoppm and reading the pages directly — same technique as the #1929/#1935 reviews. Two documents: one with a populated signature, one with the signature field cleared to '' (AC 2.4 empty-state). Scratch test file was deleted after use; git status --porcelain confirmed clean before posting this. For the React/CSS side (items 3-5) I could not get a pixel-rendered screenshot — Playwright's browser download is blocked by this sandbox's network policy (cdn.playwright.dev / playwright.download.prss.microsoft.com both 403 "blocked by default deny policy"), and there is no installable system Chromium (chromium-browser is a snap-transitional stub here). I built a static HTML harness using the actual ReportContentEditor.module.css + EditableField.module.css + tokens.css file contents (literal class names, hand-written markup matching the exact JSX tree from EditableField.tsx/ReportContentEditor.tsx) and verified it by direct CSS-rule/box-model comparison instead of a screenshot. This is a real gap in my available tooling this round, not a shortcut I chose — flagging it so it's not mistaken for a rendered check.

1. Margins / hierarchy — CONFIRMED via real render

The rendered letter (page 1) reads exactly as specified: small gray sender block tightly grouped above the recipient block (32pt gap marks the address-zone boundary), date right-aligned, Reference: REF-1 (small gray) tightly paired with the bold/larger Subject: Claim Documentation, body paragraphs separated by a clean single blank line each, closing block with generous reserved space, signature name at the end. It reads as an actual business letter, not six uniform paragraphs. letterSubject (bold, SUBHEADER_FONT_SIZE/12pt, #111827) visibly reads as a subject line, not a sixth identical block. No clipping/overlap at any margin transition.

2. Signature block, empty-signature case — CONFIRMED, one informational note

With signature: '', the render shows Sincerely, followed by the full reserved blank space (measured ~150px at 150dpi between the "Sincerely," baseline and where the footer starts, consistent with 54pt margin + the line's own ~18pt line-height) and then nothing — the block's footprint is visibly identical in size to the populated case, confirming AC 2.4 empirically, not just by code inspection.

Informational, not blocking: with the signature genuinely empty, the letter ends at "Sincerely," with trailing whitespace and no name, no rule, no visual anchor marking "sign here." It reads as acceptable rather than broken — this is exactly how a printed business letter looks before it's been physically signed, and captions/underscores were deliberately ruled out as unnecessary machinery in the original spec (§C) — but it is a hair less obviously "intentional" than the populated case. Not asking for a change; noting it for the record since the story text asked me to look specifically at this.

3/4. Step-5 preview — closing row mirrors PDF structure and chrome/content split reads correctly

Confirmed in code and via the CSS harness: JSX order is Body → Closing (readOnlyField) → Signature, exactly matching the PDF's body → closing → signature order (AC 4.2). .readOnlyLabel's font-size/weight/color (--font-size-sm/--font-weight-medium/--color-text-primary) is byte-for-byte identical to EditableField's own <label> rule (.container label in EditableField.module.css) — same three properties, same token references. That identity is what makes the caption-above-value recipe read as the panel's own established chrome pattern rather than a one-off, which is the mechanism my Option B bet actually depends on — confirmed by direct rule comparison, not assumption. Both readOnlyField rows (date, closing) use only tokens with dark-mode counterparts already defined in tokens.css Layer 3 (--color-bg-primary, --color-text-primary both flip correctly light↔dark) — no new dark-mode risk. No breakpoint-specific CSS touches .readOnlyField/.letterFields, consistent with the "reflows naturally, no media query needed" spec claim.

5. Reset button proportions — CONFIRMED via box-model trace

.container is flex-direction: column with no align-items override, so the default stretch previously forced .resetButton (a sibling flex item of .fieldWrapper, both children of .container) to stretch to the full cross-axis width — i.e. as wide as the field row above it. The 24×24 viewBox SVG at width/height: 100% then scaled the glyph to fill that entire stretched width, producing the giant/stretched X the bug described. align-self: flex-start (new) makes the button take its own intrinsic content size instead of stretching; with padding: var(--spacing-2-5) (10px) around an 18px (--font-size-lg) glyph, content-box math gives ~38px, so the pre-existing min-width/min-height: 44px (unchanged) still governs — the result is a compact ~44×44 square button, left-aligned under the field, glyph sized proportionately to body text rather than to the button's box. This resolves the bug by construction; I'm confident in it without needing a pixel render since it's a deterministic CSS box-model computation, not a subjective spacing/contrast call. :focus-visible { outline: none; box-shadow: var(--shadow-focus) } matches this project's canonical focus-ring pattern exactly.

6. Page-1 top margin (93pt) — untouched, as specified; visually acceptable

Confirmed in the diff: PAGE_TOP_MARGIN is referenced but not modified anywhere in this PR (grep across the diff shows only pass-through references). The real render confirms this reads fine in practice — the gap above "Jane Doe" (sender) measures consistently with the expected ~93pt/3.3cm and does not look like a rendering glitch; it reads as a generous but plausible business-letter top margin, matching the acceptance already recorded in my memory and this issue's own guardrail against touching page-geometry constants here. No action needed.

Verdict: APPROVED

Implementation matches the spec closely and exactly in every dimension I could verify (real PDF render for 1/2/6; direct CSS-rule/box-model comparison for 3/4/5). One informational note (empty-signature block has no visual "sign here" anchor — accepted, not a change request) and one tooling caveat (no pixel-level render available for the React/CSS side this round, see above) — neither blocks merge.

[steilerDev]

[product-owner]

Verdict: APPROVED (3 MUST FIX before merge, none blocking)

Reviewed c17d9d44 plus the locally-committed E2E follow-up d60a98b3. I wrote and amended #1932's AC set including the scope reconciliation, so this review walks every criterion individually — including the two I struck.

Local tsc/jest in this sandbox is unreliable (shared-symlink cascade), so all local runs below are provisional and CI is the authority. What I did run: applyOverrides.test.ts + coverLetterPdf.test.ts + prompts.test.ts → 149/149; realRender.test.ts → 63/63; stylelint on both changed CSS files → exit 0.

---

1. Every AC, individually

§1 — Body formatting (plain text with preserved line breaks)

AC	Verdict	Evidence
1.1	MET	applyOverrides.test.ts round-trips a body with leading spaces, trailing spaces, and a three-newline gap, then asserts byte-level survival of each. That is the exact shape a naive .trim()/collapse would alter.
1.2	MET	See the dedicated judgment below — this is mine to own.
1.3	MET	realRender.test.ts renders **bold** - dash #hash <b>tag</b> through real unmocked pdfmake and asserts .text is byte-identical both before and after getBlob(), plus exact-leaf presence via collectAllStrings. No dependency added — I diffed client/package.json, root package.json, and package-lock.json against the base: all three untouched.
1.4	MET (as replaced)	rows={6} → rows={10}. I read the whole cover-letter JSX block: no hint, helper text, placeholder, or tooltip advertising formatting support anywhere near the body field. The struck original stays struck.
1.5	MET	The one I flagged blocking-if-violated. grep -rn dangerouslySetInnerHTML client/src → zero hits repo-wide. No parser, no markup interpretation on the body path; the body reaches only a <textarea> value and a single pdfmake text node. Combined with the untouched manifests above, the negative constraint holds in full.
1.6	MET	Two sentences appended to rule 4 of REPORT_CONTENT_SYSTEM_PROMPT, naming markdown, bullets, numbered lists, HTML tags, bold/italic markers with concrete exemplars, and mandating single-blank-line paragraph separation. Critically it appends — Do NOT invent or alter amounts or dates. survives verbatim in the same rule, and #1931's pinned safety constraints are untouched. This was the only genuinely new requirement the plain-text ruling created and it is correctly the load-bearing change here.

§2 — Signature

AC	Verdict	Evidence
2.1	MET	First-class EditableField as="input" with label, ariaLabel, editedSuffix, and interpolated resetAriaLabel — identical prop shape to its five siblings.
2.2	MET	Baseline is the user's display name, not the household name. See informational note 5 on the derivation route.
2.3	MET	Change → onFieldChange('coverLetter.signature', …); reset → key removal → falls back to the generated default. Both pinned, plus the "removed key restores the derived default while sender stays overridden" case.
2.4	MET	if (coverLetter.signature) guard removed; closing (margin [0,0,0,54]) + name ([0,0,0,0]) are now unconditional and adjacent. The empty-node-reserves-a-line-height claim was verified empirically by ux-designer against a real getBlob() (+18.05pt for an empty node, identical to a non-empty one), and the rewritten coverLetterPdf.test.ts pins both the non-empty and empty-signature cases — including an explicit note that the old "omits the block" test would now have passed for the wrong reason. Good catch by whoever wrote that; a silently-vacuous replacement test was the real risk here.
2.5	MET	closing is built in buildReportContent.ts via reportT('sourceReports.coverLetter.closing') — report language. closingLabel is separate and goes through the editor's t(). The artifact-content-vs-edit-affordance split from #1909/#1924 is respected exactly.
2.6	MET both directions — see below.

§3 — Sender

AC	Verdict	Evidence
3.1	MET	if (user?.displayName) senderLines.push(user.displayName) replaces the householdName push. The test deliberately keeps a fixture carrying both householdName: 'The Smiths' and an address, so a leak of the household name would fail loudly rather than silently pass.
3.2	MET	All three degraded cases covered: user+household both null (sender '', signature '', no crash); no address (name only); user null with address only — the last one additionally asserts startsWith('\n') === false, which is the actual no-blank-line requirement rather than a proxy for it.
3.3	MET	Existing sender override behaviour preserved end-to-end; the realRender sender-override test still passes.
3.4	MET	Availability gating untouched — no diff to the includeCoverLetter logic or the disabled hint.

§4 — Letter layout

AC	Verdict	Evidence
4.1	MET	Margins 4/32/20/4/16/32/54/0, right-aligned date, new bold letterSubject. Order unchanged (it already matched); the change is hierarchy, as specified. Paragraph separation resolved per spec §B: a blank line stays one blank line at body line-height, no per-paragraph node split. I accept that — it honours §1's own "regression guard, not a feature build" framing and avoids exactly the body restructuring AC 7.2 exists to catch.
4.2	MET, but unpinned — see MUST FIX 2	Preview order now mirrors the PDF exactly: sender → recipient → date → reference → subject → body → closing → signature.
4.3	MET	No page cap on the letter; pageBreak: 'after' still terminates it, so pdfmake paginates rather than clips. ~42pt net margin increase, no geometry constant touched.
4.4	MET	ux-designer spec posted before the layout work landed, and the implementation matches it line for line (I diffed the spec's content array against coverLetterPdf.ts — identical).
4.5	MET	subjectLabel/reference prefixes and dateLine/closing all resolve through the reportT threaded into buildCoverLetterContent; only edit affordances use interface t().

letterSubject's #111827 is not a token violation — PDF_STYLES lives in .ts, is outside stylelint's client/src/**/*.css scope, and pdfmake cannot consume custom properties. Same accepted pattern as every other colour in that object. Correctly added to pageGeometry.ts, not merge.ts, and it reuses SUBHEADER_FONT_SIZE rather than hand-writing 12. The post-#1939 handoff constraints were all respected: no geometry constant touched, overviewPdf.ts untouched.

§5 — Reset button

AC	Verdict	Evidence
5.1	MET	svg { width/height: var(--font-size-lg) } replaces 100%.
5.2	MET	min-width: 44px; min-height: 44px retained verbatim. align-self: flex-start on a column flex parent constrains the cross axis (width) only — height is unaffected, so the hit area survives.
5.3	MET	align-self: flex-start is precisely the fix for the full-width stretch; the button stays a direct child of .container, which the new unit test pins structurally.
5.4	MET	New :focus-visible { box-shadow: var(--shadow-focus) }. Token is defined in both themes (tokens.css:265 light, :827 dark) — the AC asked for both and both exist. This was previously missing entirely.
5.5	MET	EditableField.tsx is not in the diff at all — accessible name, title, edited dot, and SR hint are structurally unchanged.
5.6	MET	Shared component only; no call-site override anywhere.

The accompanying unit test is honest about its own limits (jsdom has no layout engine, so it pins class presence and DOM position rather than pretending to measure pixels) and says so explicitly, citing the #1929 postmortem. That is the right call — a fake computed-dimension assertion would have been worse than no test.

§7 — Tests

7.1 MET · 7.2 MET · 7.4 MET · 7.3 in flight (see item 6).

---

2. Is AC 1.2 met by a line-count-plus-spacing proof?

Yes. I own the strengthening and I judge this satisfies it.

My bar was explicit: "asserted against a real render (pdfmake's resolved layout), not merely against the string content of the emitted text node" — because node.text === 'a\nb' proves nothing about the page. What shipped clears that bar by a wide margin:

• It reads .positions off the node after a real createPdf()/getBlob(). Those top values only exist post-layout — they cannot be produced by the pre-render content tree.
• It asserts exactly 4 rendered lines for a 4-segment body. If pdfmake collapsed the blank line → 3 → fail. If a future change reflowed the body into per-token inline runs (the Report PDF layout breaks: usage column overflows the page, rows split across page breaks, running header clipped #1929 wordBreak technique, which is the specific silent breaker AC 7.2 names) → \n handling is destroyed → fail. The count is genuinely discriminating, not incidentally satisfied.
• It asserts all three inter-line gaps are equal and non-zero, which is what actually proves the blank line occupies one ordinary line — neither collapsed to zero nor doubled.
• The lines are deliberately short so word-wrap can't add a phantom line, and the test says so.

The one thing it does not do is reconstruct per-line text. That does not matter here, because the body is a single text node whose .text is separately asserted byte-identical (AC 1.3's test does this both pre- and post-render), and pdfmake lays a single node's text out in order. Byte-identical content + exactly-4-lines + uniform spacing is sufficient to conclude each typed segment became its own line at its own position.

The helper is also right about the mechanism, and documents why: ._inlines drains to [] via .shift() during layout, so .positions is the only post-render signal with the correct cardinality. That was verified empirically before the helper was written rather than assumed. This is materially stronger than the content-tree string check I rejected. AC 1.2 met.

3. AC 2.6, both directions — and the pinned assertion

Both directions met.

• Signature edited, then sender edited → signature survives. applyOverrides.ts now checks overrideKey.coverLetter.signature in overrides first and only falls through to the sender recompute in the else branch. Pinned at unit level (including a key-insertion-order pair, which correctly proves the guard isn't accidentally order-dependent) and at real-render level via an it.each over both key orderings.
• Sender edited, signature never touched → signature still recomputes from the new sender's first line. Pinned by the "removing the signature override key and re-applying restores the sender-derived default" test, which keeps sender overridden while dropping signature — the exact discriminating case.

The realRender pin uses toContain on the string array (exact element equality, not substring), so "sender happens to contain Jane Doe" is correctly distinguished from "signature was recomputed to Jane Doe." That distinction is subtle and the test comment nails it.

On "updated, not deleted": realRender.test.ts:1057 — "overriding coverLetter.sender changes the rendered signature too (recomputed by applyOverrides)" — was kept intact and still passes. It was not deleted, which was my actual concern, and it is still factually correct: it overrides only sender, which is now precisely the fallback branch. So the intent of my requirement is honoured. It was not reworded, though, and its unqualified title now reads as an unconditional rule that the AC 2.6 test sitting 20 lines below directly contradicts. Recorded as informational note 4, not a MUST FIX — the assertion is right, only its name is now under-qualified, and the AC 2.6 test is the real guard against anyone restoring the unconditional recompute.

4. AC 6.1 / 6.2 — does Option B satisfy #1925?

Yes, and the split reads as deliberate.

ux-designer rejected Option A (translate the caption via reportT) on the correct grounds: every sibling caption in that panel — Sender, Recipient, Subject, Body, Signature — is an EditableField <label> in the interface language. Translating only the date and closing captions would have made them the single inconsistent caption in the panel, which is a direct violation of AC 6.2's own "consistent with the other captions in the same panel" clause. Option A would have fixed 6.1 by breaking 6.2.

The real defect was never the caption's language — it was Datum: February 15, 2026 rendering as one visually-joined sentence fragment. The fix restructures the row from flex-direction: row to column and drops the literal :, so the caption becomes its own line styled exactly like every other caption. AC 6.1 satisfied structurally (no single rendered line can mix languages when caption and value are separate flex-column items). AC 6.2 satisfied on two independent signals: the label now matches EditableField's <label> recipe exactly (--font-size-sm, --font-weight-medium, --color-text-primary), and the row keeps --color-bg-primary — flatter than the --color-bg-tertiary of the editable input boxes, so read-only rows still read as distinct from editable ones.

On the adjacent-different-languages question: yes, closingLabel ("Grußformel") and closing ("Sincerely,") will legitimately sit one above the other in different languages, and that is correct, not broken. The label is the app telling you what this is; the value is the letter's own text. That is the same relationship every other row in the panel already has — a German-interface user editing an English report sees "Betreff" above English subject text today. Stacking them vertically is what makes it read as caption-and-artifact rather than as one broken sentence. This is #1909's rule applied consistently, and it now applies to two rows instead of zero.

5. #1925 bookkeeping — and a correction to my own §6

My §6 transcription was incomplete, and I'm recording that as my error. #1925 has six ACs, not four. I carried its AC1→6.1, AC2→6.2, AC4→6.3, AC6→6.4, and dropped its AC3 and AC5. Checking all six rather than the four I carried:

#1925 AC	Status
1 — no line mixes the two languages	MET (= 6.1 above)
2 — caption distinguishable as chrome	MET (= 6.2 above)
3 — PDF date stays a bare, label-free, report-language date	MET in substance. The "unchanged" clause is legitimately superseded by #1932 §4 (right-alignment is an authorised change), but the load-bearing part holds: { text: coverLetter.dateLine, alignment: 'right' } — no caption, no prefix.
4 — dateLine still via reportFormatters.formatDate	MET (= 6.3), untouched.
5 — unit pins on both the editor and PDF sides	PARTIAL → MUST FIX 3. PDF side is pinned: coverLetterPdf.test.ts asserts c.text === 'date(2026-03-01)' by exact equality, so any prefix reappearing fails. Editor side pins only what pre-existed (the value renders, it isn't an input) — nothing pins the colon-free caption that is #1925's fix.
6 — locale parity on dateLabel	MET (= 6.4).

Parity verified programmatically across all 17 namespaces × en/de: zero missing, zero extra, budget at 1055/1055 as claimed. closing correctly sits under coverLetter (artifact) and signatureLabel/closingLabel under editable (chrome) — the namespace placement encodes the content/chrome split, which is the right structural choice. No glossary entry needed; neither is a domain term.

Action taken: #1925 closed as a duplicate of #1932, board → Wont-Do, with a comment enumerating all six ACs and where each landed. Duplicate closure transfers ownership; the residual AC 5 pin is tracked as MUST FIX 3 on this PR, where it will actually be acted on.

6. AC 7.3 ordering

Your ordering is right — don't change it. AC 7.3 is part of #1932's AC set, so it belongs in the PR that closes the story, not in a follow-up issue. Deferring it would let #1932 merge with a documented AC unmet, and my §7 exists precisely so the test obligations are atomic with the feature. Landing d60a98b3 on this PR before merge is the correct call, and catching it yourself is the right outcome.

I reviewed d60a98b3: Scenarios 21 (desktop/light, genuine two-paragraph body), 22 (mobile/dark, same flow), 23 (AC 2.6 signature-then-sender ordering), 24 (reset, doubling as a live guard that the CSS-only §5 fix didn't break the reset locators). That covers everything 7.3 asks for plus the two interactions I'd have asked for anyway. Note it pairs viewport with theme (desktop+light, mobile+dark) rather than running a full 2×2 — consistent with the existing convention in that spec file, and the intent (both viewports, both themes) is covered. Accepted.

Two conditions on the merge, both mechanical:

1. Re-run scripts/ci-wait.sh against the head after pushing d60a98b3. The green on c17d9d44 does not cover a commit that changes the E2E suite. Watch the known pre-existing shard-5 flake and don't let it be mistaken for a new failure.
2. Delete client/src/lib/reportPdf/__scratch_ux1951.test.ts before touching the index again. The UX spec says the scratch render test was "deleted afterward — not committed"; it wasn't deleted. It is untracked and not gitignored, so a git add -A would sweep it straight into the follow-up commit.

---

MUST FIX before merge (non-blocking)

1. German closing carries a comma it shouldn't. de/budget.json: "closing": "Mit freundlichen Grüßen,". Standard German business-letter convention (DIN 5008) puts no comma after the Grußformel; the English "Sincerely," correctly takes one. This is a one-character change in de only, and it matters more than it looks: the entire premise of this story is that a bank employee reads the first page as professional correspondence, and this is the German-language version of that page. → translator.
2. The new Closing read-only row has zero unit coverage. Deleting that JSX block leaves every test in ReportContentEditor.test.tsx green, so AC 4.2's preview-mirroring is unpinned. Add an assertion that the row renders closingLabel plus content.coverLetter.closing, positioned between the Body and Signature fields. → qa-integration-tester.
3. Report wizard preview: cover-letter date caption uses the UI language beside a report-language date #1925 AC 5's editor-side pin is missing. Add an assertion that the date caption renders without a trailing : (exact-text match on the label), so the concatenated Datum: February 15, 2026 line cannot drift back. Same file as (2); both ride along with the E2E commit. → qa-integration-tester.

Informational (no action requested)

4. realRender.test.ts:1057 kept but not reworded. Correct and passing, but its unqualified title now describes what is only the fallback branch. A "…when signature has not itself been overridden" qualifier would remove the ambiguity for the next reader. The AC 2.6 test is the real guard, so this is cosmetic.
5. Signature is still derived as sender.split('\n')[0], not from user.displayName directly. AC 2.2 asks for a default derived from the user's identity; today it is derived from the sender's first line, which is the display name only because §3 puts it there. In the theoretical degraded path (user null, household address present) the signature would be the street address. Unreachable in practice — users.display_name is notNull with minLength: 1, and user is a useMemo dependency so a transient null recomputes once auth resolves. Flagging the indirection, not requesting a change.
6. Scope discipline was clean. No letterhead/logo, no stored templates, no change to which report types offer a cover letter, no change to the report table, no new dependency, no geometry constant touched, overviewPdf.ts untouched. Nothing in this PR exceeds Cover letter overhaul: formatted body, editable signature block, personal sender, professional layout #1932's documented scope.

---

Verdict: APPROVED — all 40 acceptance criteria met, including both criteria I struck as vacuous, whose retained negative constraints hold in full. AC 1.5 in particular — the one I said reviewers should treat as blocking — is clean on every axis I could check. Three MUST FIX items before merge, none of which affects a functional criterion; two of them are the same test file and land with the E2E commit.

#1932 stays out of Done until UAT: the user rejected the previous output by looking at a generated PDF, and this story is answering that rejection. It should be accepted the same way — a human reading a real exported cover letter in both EN and DE.

[steilerDev]

[product-architect]

Architecture review — PR #1951 (#1932, cover letter overhaul)

Verdict: CHANGES_REQUIRED — one HIGH finding (a shipped E2E scenario that cannot pass). Everything else below is non-blocking.

Reviewed against issue #1932 (amended body), the ux-designer layout spec (comment 5160362068), and the post-#1939 state of client/src/lib/reportPdf/. I verified the load-bearing mechanical claims against source rather than taking them on trust — notes inline.

---

HIGH-1 (blocking) — E2E Scenario 24 seeds a source with neither contactAddress nor reference, so the cover letter is never enabled

e2e/tests/budget/reportWizardEditableContent.spec.ts (Scenario 24 seed):

sourceId = await createBudgetSourceViaApi(page, {
  name: `${testPrefix} ResetSig Source`,
  totalAmount: 10000,
});

ReportWizardPage.tsx L279 auto-enables the cover letter with
setIncludeCoverLetter(Boolean(r.source.contactAddress || r.source.reference)), and the spec's own
reachStep5() helper never clicks includeCoverLetterCheckbox. createBudgetSourceViaApi
(e2e/fixtures/apiHelpers.ts L52-55) defaults only sourceType/status — it supplies neither field.

With neither present, content.coverLetter is null, [class*="coverLetterCard"] never mounts, and
wizard.letterField('signature') resolves to zero elements. The scenario then:

• passes await expect(wizard.resetButtonFor(signature)).not.toBeVisible() vacuously (0 elements is trivially not-visible), then
• hangs and fails on await signature.inputValue() / await wizard.editField(signature, ...) → field.fill() on a 0-element locator.

Every other cover-letter-touching scenario in this file — including Scenarios 21, 22 and 23 added in the
same commit — seeds both fields for exactly this reason. Scenario 24 is the only one that doesn't.
This also means it accidentally exercises AC 3.4's disabled path while asserting the enabled one.

Fix: add contactAddress: '1 ResetSig St, Testville' and reference: 'Ref-RESETSIG' to the seed,
matching Scenarios 21-23. (A deliberate AC 3.4 scenario asserting the disabled hint would be a genuinely
useful addition, but it must not try to edit a signature field.)

Corroboration, not proof: E2E shards 2/16 and 5/16 have already gone red on d60a98b3; per-test logs
are not yet retrievable while the run is in flight. Shard 5 has a known unrelated flake history, but
shard 2 does not, and the static reading above is unambiguous on its own. Scenario 24 runs in all three
viewport projects, so expect it in more than one shard.

---

Answers to the specific questions raised

1. pageGeometry.ts direction constraint and letterSubject's home — constraint held; home is correct.

pageGeometry.ts has no import from merge.ts — the only occurrences of "merge" in the file are prose
in the header comment and the DEFAULT_LINE_HEIGHT note. Edge direction is intact
(pageGeometry ← merge), and merge.ts L114-119 still re-exports.

Adding letterSubject to PDF_STYLES where PDF_STYLES lives is right — putting it anywhere else
would recreate the two-locations problem #1939 solved. It does sharpen the misnomer: letterSubject
is the first PDF_STYLES entry with no geometry consumer at all, so the module is now
"geometry constants + the style dictionary". I do not think a split is warranted yet. The clean shape
when it is would be a pdfStyles.ts importing from pageGeometry.ts and re-exported by merge.ts
(direction pageGeometry ← pdfStyles ← merge). Suggested trigger for that split: the second
geometry-unconsumed style. Worth one line in the file header so the next author has the rule.

2. HEADER_ROW_HEIGHT_MAX — confirmed absent, and the pageBreak: 'after' guarantee holds.

grep -rn HEADER_ROW_HEIGHT_MAX client/src server/src shared/src returns only its definition
(overviewPdf.ts:353) and realRender.test.ts. coverLetterPdf.ts never touches it. The
drift signal did not fire.

The page-break guarantee is genuine: merge.ts L114-119 pushes ...buildCoverLetterContent(...) then
...buildOverviewContent(...) into one flat content array, and the letter's terminal node carries
pageBreak: 'after', so the table's first node always starts on a fresh page. One pre-existing cosmetic
edge, not introduced here and not worth fixing speculatively: the { text: '', pageBreak: 'after' }
sentinel reserves a full ~18pt line (by the very measurement this PR relies on for the signature block),
so a letter ending within ~18pt of the bottom margin spills the sentinel to page N+1 and pushes the table
to N+2, leaving a near-blank page. This PR raises the letter's margin budget ~120pt → ~162pt, which nudges
that window without creating it. A comment on the sentinel would be enough.

3. AC 1.2's real-render proof — sufficient for the AC as written; one cheap hardening available.

I verified the .positions mechanism against node_modules/pdfmake/js/LayoutBuilder.js rather than
relying on the summary. node.positions.push(positions) (L1183) sits inside processLine's per-line
while loop with node bound to the text node being laid out (node._inlines, buildNextLine(node)),
and decorateNode() (L1279) initialises node.positions = [] per node. So positions.length is
exactly the rendered visual-line count for that node. The _inlines-drains-to-[] finding is
corroborated by L1174's node._inlines.unshift(...) on the reflow path — it is a consumed queue.

Each failure mode AC 7.2 names fails loudly, not silently:

• reflow into per-token inline runs → findBodyItem's typeof text === 'string' predicate matches nothing and throws a named error;
• blank line collapsed → 3 lines, not 4;
• blank line doubled → 5, or a non-uniform gap;
• blank line at zero height → the blank→line-3 gap breaks uniformity.

The residual gap is real but narrow, and I'd rather name it than wave it through: the test never asserts
the body node's .text equals the override (only .includes() on the first line) and never reconstructs
per-line text. A transformation preserving line count and spacing while rewriting content within
lines — normalising \n\n to \n \n, or trimming per-line leading spaces — would pass. AC 1.2's wording
("every line break and blank line ... in the same order and at the same positions") is satisfied by
count + uniform spacing; the stronger claim "the rendered text is byte-identical" is not proven.

Cheap hardening (informational): add expect(bodyItem['text']).toBe(body) to the AC 1.2 test — the
sibling AC 1.3 test already does exactly this. That closes the gap at one line's cost.

4. Blast radius of required closing — genuinely client-only, but by accident of placement.

Verified: ReportContentCoverLetter is referenced only under client/src/lib/reportContent/ and
client/src/lib/reportPdf/. grep -rn coverLetter shared/src server/src returns nothing — neither
workspace models the cover letter at all. e2e/ and docs/ construct no such fixture. The sole producer
is buildReportContent. So a client-only typecheck was sufficient here; there is no hidden server or
shared consumer a tsc pass would have missed.

That is the right boundary — closing is a client-derived presentation value that never crosses the API.
See MEDIUM-2 for the documentation consequence.

5. user optional on buildReportContent — the asymmetry is the smell, not the optionality.

Production risk today is effectively zero: one call site (ReportWizardPage.tsx L326), correctly wired,
pinned by an exact-string unit test (ReportWizardPage.test.tsx, "threads the mocked user displayName").

But within options, household is required-nullable and user is user?:, and there is no design
reason for the difference — both feed the same sender string, and user is now the more load-bearing
of the two (it supplies both the name and the signature default). A future call site can omit user,
get a silently address-only sender, and compile clean: precisely the failure AC 3.1 exists to prevent.

I am not requesting the change now — making it required-nullable churns ~47 option-passing call sites
for a type-level nicety on a codebase with one production caller, and that churn has its own risk. Record
it as the intended end state for whoever is next in this file: user: { displayName: string } | null,
ideally Pick<UserResponse, 'displayName'> rather than a re-declared inline shape.

6. AC 1.5 is enforced by review only — and that is cheaply fixable.

eslint.config.js has no react/no-danger, and dangerouslySetInnerHTML appears nowhere in
client/src today. Enabling react/no-danger for the client is therefore a zero-diff change that turns
half of AC 1.5 into a CI gate instead of a standing convention — exactly the "let's just add marked"
PR the PO is worried about would trip it. I own eslint.config.js; I will take this as a follow-up
rather than bolt it onto this PR.

The other half — "no new markdown/rich-text dependency" — stays review-only. A dependency allowlist is
over-engineering at this scale, but the constraint should live somewhere durable (Architecture page)
rather than only in a closed issue's AC.

7. E2E inspection — HIGH-1 above, plus two low notes in the findings list below.

---

MEDIUM (non-blocking)

MEDIUM-1 — AC 1.6's guarantee is prompt-level only; nothing enforces it server-side.
prompts.test.ts pins that the instruction exists, which is all AC 1.6 asks. But the response
validator truncates length and does not strip markup, and the client renders letterBody literally into
a bank-facing PDF. A non-compliant or drifting model still puts ** on the page. Follow-up worth filing:
a cheap sanitisation pass on letterBody at validation time (strip **/__, leading - /# , <tag>)
would turn a soft guarantee into a hard one. Out of scope here; flagging so it is not lost.

MEDIUM-2 — wiki not updated for a changed API output guarantee.
wiki/API-Contract.md documents POST /api/source-reports/generate-content (L3785-3830). This PR adds a
new, contract-adjacent guarantee about letterBody's format — the client PDF path depends on it — and
the page says nothing about it. Per the wiki-update discipline this should have landed in the same PR,
not as a review catch. Add one bullet under Notes next to the length-limits bullet:

• letterBody is constrained by the prompt to plain prose — no markdown, bullets, numbered lists, HTML tags, or bold/italic markers; paragraphs separated by a single blank line (Cover letter overhaul: formatted body, editable signature block, personal sender, professional layout #1932). The client renders it literally into the PDF and performs no markup parsing.

While there, one clarifying sentence that the cover letter's closing, dateLine and signature are
client-derived and deliberately absent from the response shape, so a future backend author does not
add a server-side closing field. No Schema page change — ReportContentCoverLetter is a client
presentation type with no persistence.

---

LOW / informational

• letterSubject reuses SUBHEADER_FONT_SIZE, coupling two unrelated sizes. That constant is
  load-bearing in headerFootprint()'s arithmetic (pageGeometry.ts L144). Changing the report
  subheader's size for footprint reasons now silently resizes the letter's subject line, and vice versa.
  reportPdf geometry hygiene: bounds that name their own scope (HEADER_ROW_HEIGHT_MAX, char-advance scope, channel enumeration, PDF_STYLES relocation) #1939's rule was "don't keep two drifting copies of one constant" — but these are two constants that
  happen both to be 12pt, which is the opposite problem (false sharing). A one-line
  const LETTER_SUBJECT_FONT_SIZE = SUBHEADER_FONT_SIZE; with a comment stating whether the equality is
  intentional or coincidental would settle it. The UX spec directed the reuse, so this is a note for the
  next edit, not a rework request.
• Scenario 22 asserts nothing dark-mode-specific. It sets data-theme="dark" (correct convention —
  matches ThemeContext.tsx L68 and the budget-categories.spec.ts precedent) and then re-runs the
  Scenario 21 flow. That proves the flow survives the attribute, which is close to nothing about dark
  mode. AC 7.3's "in both light and dark mode" is satisfied literally; a computed-color or contrast
  assertion on the new .readOnlyField rows would make it real.
• German closing has a trailing comma. client/src/i18n/de/budget.json: "Mit freundlichen Grüßen,".
  German convention takes no comma after this salutation (unlike English "Sincerely,"). Small, but it
  is the sign-off on a document handed to a bank. Suggest "Mit freundlichen Grüßen".
• PR body is stale. It states "No new E2E coverage was added for this story — deferred" and leaves
  the E2E checkbox unticked, but commit d60a98b3 adds Scenarios 21-24 (+309 lines) and a POM locator.
  The body feeds the promotion PR's change inventory — please correct it before merge.
• EditableField.module.css's outline: none + box-shadow: var(--shadow-focus) matches the
  established repo-wide pattern (shared.module.css .btnPrimary/.input/etc.), and --shadow-focus
  is defined in both light (L265) and dark (L827) blocks. AC 5.4 satisfied; no new finding.

---

Verified clean

• Direction rule: pageGeometry.ts does not import merge.ts. ✅
• Drift signal: HEADER_ROW_HEIGHT_MAX unreferenced by coverLetterPdf.ts. ✅
• AC 2.5 / 4.5: closing resolved via reportT('sourceReports.coverLetter.closing') in
  buildReportContent.ts; the editor's closingLabel caption goes through interface t(). The
  artifact-content-vs-edit-affordance split from feat(reports): editable HTML report preview with on-demand PDF export #1909/feat(reports): report table cleanup — shared footnotes, deposit labels, claim metadata, total-only summary, usage area (#1923) #1924 holds. ✅
• AC 6.4 locale parity: closing, closingLabel, signatureLabel present in both en and de. ✅
• AC 2.6 in applyOverrides.ts: signature-override check precedes the senderChanged fallback and is
  order-independent (property presence, not iteration order). Correct, and the reset path — key removed →
  falls back to sender-derived — is covered. ✅
• No orphaned CSS: .dateLineField/.dateLineLabel/.dateLineValue have zero remaining references
  anywhere in client/src or e2e/. ✅
• _inlines/.positions claim: independently confirmed against LayoutBuilder.js (see Q3). ✅
• Trailers: both commits carry the required co-authors; Trailer Check green. Static Analysis
  (typecheck + build + audit) green on d60a98b3 — the earlier ts-jest/typecheck gap is closed on CI. ✅

---

Re-request review once HIGH-1 is fixed. Everything under MEDIUM/LOW can ride as follow-ups; I will take
the react/no-danger rule and the API-Contract wiki bullet myself unless you would rather fold the wiki
bullet into this PR (mildly preferable, since it documents a guarantee this PR introduces).

[steilerDev]

[product-owner]

Follow-ups filed from the architecture review

Both non-blocking findings from the architect's review are now tracked. Neither blocks this PR.

Issue	Finding	Priority	Board
#1952	MEDIUM-1 — the plain-prose guarantee is prompt-level only; validateGenerateReportContentResult strips nothing	Should Have	Todo
#1953	LOW — letterSubject false-shares SUBHEADER_FONT_SIZE with headerFootprint()'s arithmetic	Could Have	Backlog (blocked by #1932)

Product rulings made in the issues, so they are not relitigated

#1952 — strip, do not reject. The architect left three options open (strip / reject / accept prompt-level-only). Ruled strip, for four reasons: one LLM call produces the subject, body and all per-invoice descriptions, so rejecting over two asterisks discards unrelated correct output and may fail identically on retry; the validator's existing policy for these very fields is already coercion (it truncates rather than throws), so a harsher policy for a milder violation would be incoherent; the fields are human-editable in the preview, which argues for repair-and-move-on rather than hard failure; and accepting prompt-level-only simply leaves the known-wrong output reachable. Scope covers letterBody, letterSubject and descriptions[] — all three render literally into the PDF through the same validator. The ACs put as much weight on false-positive guards (Rechnung #2024-117, Pos. 3 - Dachstuhl, Beträge < 500 EUR must survive byte-identical) as on the stripping itself; a mangled reference number is worse than the markup it replaced.

Note for whoever picks it up: the wiki bullet this PR adds ("instructed but not enforced") is the honest interim statement. #1952 AC 4.1 amends it rather than deleting it.

#1953 — the equality is coincidental; split with an independent literal, not an alias. The UX spec did direct the reuse, so this needed settling rather than leaving to taste. Its stated rationale (comment 5160362068 line 52) is "don't hand-write fontSize: 12 as a second copy of that constant" — an argument against a magic literal, not a claim that the two sizes are the same thing. The spec's design reasoning for the subject line (line 35, "bold + bumped size makes it read as a subject") stands alone and never references the running header. So const LETTER_SUBJECT_FONT_SIZE = SUBHEADER_FONT_SIZE; is explicitly ruled out — it fixes the name while preserving the exact coupling that is the problem.

#1953 also records the architect's PDF_STYLES module-split trigger so it does not evaporate with this review: letterSubject is the first style entry with no geometry consumer, a split is not warranted now, and the trigger is the second such entry — target shape pageGeometry <- pdfStyles <- merge. AC 3.1 puts that in the file header where the next author will actually read it.

Deliberately not filed

• German trailing comma — already fixed in this PR ("Mit freundlichen Grüßen", no comma, per DIN 5008). Flagged independently in my own review; no issue needed.
• react/no-danger for AC 1.5 — the architect explicitly took this as its own follow-up (eslint.config.js is its file). Not filed yet as of this comment — latest issue in the repo is pageGeometry: letterSubject false-shares SUBHEADER_FONT_SIZE with the running-header footprint math — split the constant, record the PDF_STYLES split trigger #1953. @product-architect, please file it so AC 1.5's "no dangerouslySetInnerHTML" half becomes a CI gate rather than a standing convention; I have deliberately left it alone to avoid a duplicate. The other half ("no new markdown/rich-text dependency") stays review-only and should be recorded on the Architecture page as the architect suggested, since a closed issue's AC is not a durable home for a standing constraint.

PR body

The stale "No new E2E coverage was added — deferred" line is already corrected on the current body: it now lists Scenarios 21-24 with the E2E checkbox ticked. Checked the two places it could have propagated — there is no open beta -> main promotion PR, and RELEASE_SUMMARY.md predates the report-wizard work entirely and will be regenerated by docs-writer at the next promotion. Nothing downstream picked up the stale claim.

[github-actions]

🎉 This PR is included in version 2.13.0-beta.48 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀