From c17d9d44f2897e376560e312c5a27f7e81625bd0 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Sun, 2 Aug 2026 23:50:42 +0200 Subject: [PATCH 1/3] feat(reports): rebuild the cover letter as a business letter (#1932) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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; #1931's pinned safety constraints are untouched). - `signature` becomes a first-class editable field; an explicit signature edit is no longer silently overwritten when the sender is edited afterward. - 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. - Letter hierarchy via varied margins, a right-aligned date, and a bold/larger subject style. - 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 44x44 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. Note: no new E2E coverage was added for this story (deferred); will flag in the PR description. Fixes #1932 Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude backend-developer Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude translator Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude ux-designer Co-Authored-By: Claude product-owner --- .claude/agent-memory/product-owner/MEMORY.md | 3 +- .../product-owner/bank-report-wizard.md | 60 ++++++ .claude/agent-memory/translator/MEMORY.md | 11 + .claude/agent-memory/ux-designer/MEMORY.md | 2 + .../ux-designer/feature-spec-history.md | 9 + .../pdfmake-rendering-verification.md | 30 +++ .../EditableField/EditableField.module.css | 10 +- .../EditableField/EditableField.test.tsx | 47 +++++ .../reports/ReportContentEditor.module.css | 16 +- .../reports/ReportContentEditor.test.tsx | 110 ++++++++++ .../reports/ReportContentEditor.tsx | 31 ++- client/src/i18n/de/budget.json | 5 +- client/src/i18n/en/budget.json | 5 +- .../lib/reportContent/applyAiContent.test.ts | 1 + .../lib/reportContent/applyOverrides.test.ts | 88 ++++++++ .../src/lib/reportContent/applyOverrides.ts | 15 +- .../reportContent/buildReportContent.test.ts | 72 ++++++- .../lib/reportContent/buildReportContent.ts | 11 +- .../lib/reportContent/overrideKeys.test.ts | 9 +- client/src/lib/reportContent/overrideKeys.ts | 1 + client/src/lib/reportContent/types.ts | 5 +- .../src/lib/reportPdf/coverLetterPdf.test.ts | 60 +++++- client/src/lib/reportPdf/coverLetterPdf.ts | 42 ++-- client/src/lib/reportPdf/merge.test.ts | 1 + client/src/lib/reportPdf/pageGeometry.ts | 5 + client/src/lib/reportPdf/realRender.test.ts | 193 ++++++++++++++++++ .../ReportWizardPage.aiGeneration.test.tsx | 19 ++ .../ReportWizardPage.test.tsx | 64 ++++++ .../ReportWizardPage/ReportWizardPage.tsx | 5 +- .../services/budgetExtraction/prompts.test.ts | 22 ++ .../src/services/budgetExtraction/prompts.ts | 2 +- 31 files changed, 885 insertions(+), 69 deletions(-) diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index d5577bd61..c1b8028b2 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -33,11 +33,12 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) (budget/invoice) and [standalone-diary-bugs.md](standalone-diary-bugs.md) / [standalone-photo-stories.md](standalone-photo-stories.md). Growing budget/invoice cluster (20+) → propose new "Budget/Invoice UX Polish" epic next planning cycle. Auto-itemize is standalone (no parent epic): stories #1545-#1547 + bug fixes. +- **#1932 scope ruled by user 2026-08-02**: cover-letter body = **plain text with line breaks**, no markdown/WYSIWYG/new dep; bold+lists out of scope. Premise correction: **pdfmake already honours `\n`** (`TextBreaker.js` L30-34/53-58) so §1 became regression guards + the AI-prompt guard (AC 1.6). ACs 1.1-1.6 amended, 1.4 struck, 1.5 kept as negative constraint, **AC 2.6 added** (`applyOverrides` signature-from-sender recompute), paragraph spacing moved §1→§4.1. Blocked-by = only #1939 (PR #1948). Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1932 user scope ruling". - Budget/invoice batches: #1369-#1373 (2026-04-28), #1389-#1390 (2026-04-29), #1401 (2026-05-10), #1421-#1425 (2026-05-15), #1439-#1441 (2026-05-17), #1553 (2026-05-22) - Auto-itemize: #1545/#1546/#1547 mini-epic (2026-05-21), #1600 (2026-05-26), **#1833 duplicate budget lines on commit retry (2026-07-07)** - Diary: #1426 critical photo data loss (2026-05-15) - Photo: #1723 lightbox picker UX (2026-06-16) -- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question) — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **but #1943** (bug, **Must Have**, Todo, 2026-08-02) — `handleUseCaseChange` never clears `report`/`sourceId`, so budget-overview→claim carries a stale report and can embed **quotation-tier docs in a claim PDF**, reaching #1930 AC2's forbidden outcome by a route AC2 doesn't cover; ruled: clear `sourceId` too — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **#1888 body re-scoped to indicator presentation only at review time** (it was still stale), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **PR #1945 (#1943) review follow-ups filed 2026-08-02**: **#1946** in-flight AI generation survives a use-case change (bug, **Must Have**, Todo — product ruling: widen `guardedUpdate`'s dirty predicate to include `isGeneratingAi`, confirm invalidates via token, cancel lets it finish) and **#1947** `ReportWizardPage` `useReducer` refactor (tech-debt, Should Have, Backlog, blocked-by #1946, filed separately from #1912 on purpose). **M1 was fixed inside PR #1945, not filed** — precedent: a finding that defeats an AC of the story under review belongs in that story's PR regardless of reviewer severity. #1943 AC4 reworded + AC5 enumeration completed (`skippedDocuments`/`aiError` ruled CLEAR, carried as #1946 AC9/AC10); #1933 AC2.1/2.7 corrected (invoice row has no mobile card). See [bank-report-wizard.md](bank-report-wizard.md) §"#1945 review follow-ups" and §"#1933 ACs 2.1/2.7" — the latter records a **recurring AC-writing failure mode** (ACs that misdescribe reality fail correct implementations at UAT). **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". **#1931 PR #1944 APPROVED round 1, 2026-08-02** — all ACs met **except 3.2/3.3, deliberately NOT claimed**: they assert live-model output quality, which a mocked LLM cannot verify. Ruling: **merge is a code gate, Done is an acceptance gate** — PR merges, story stays out of Done until a human reads real EN+DE output with `LLM_*` set; UAT scenarios posted on #1931; failure → reopen #1931, don't file a follow-up. Contrast #1909 AC 4.6: an unverifiable AC **with** a substitute assertion may be waived as a documented deviation; **without** one it goes to UAT. "Mit KI verbessern" accepted for AC 2.3. **#1917 L3 struck** (verified fixed); rest of #1917 open, **`KI` glossary entry still #1917's** — `glossary.json` untouched by #1944. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1931 reviewed". +- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question) — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **but #1943** (bug, **Must Have**, Todo, 2026-08-02) — `handleUseCaseChange` never clears `report`/`sourceId`, so budget-overview→claim carries a stale report and can embed **quotation-tier docs in a claim PDF**, reaching #1930 AC2's forbidden outcome by a route AC2 doesn't cover; ruled: clear `sourceId` too — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **#1888 body re-scoped to indicator presentation only at review time** (it was still stale), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **PR #1945 (#1943) review follow-ups filed 2026-08-02**: **#1946** in-flight AI generation survives a use-case change (bug, **Must Have**, Todo — product ruling: widen `guardedUpdate`'s dirty predicate to include `isGeneratingAi`, confirm invalidates via token, cancel lets it finish) and **#1947** `ReportWizardPage` `useReducer` refactor (tech-debt, Should Have, Backlog, blocked-by #1946, filed separately from #1912 on purpose). **M1 was fixed inside PR #1945, not filed** — precedent: a finding that defeats an AC of the story under review belongs in that story's PR regardless of reviewer severity. #1943 AC4 reworded + AC5 enumeration completed (`skippedDocuments`/`aiError` ruled CLEAR, carried as #1946 AC9/AC10); #1933 AC2.1/2.7 corrected (invoice row has no mobile card). See [bank-report-wizard.md](bank-report-wizard.md) §"#1945 review follow-ups" and §"#1933 ACs 2.1/2.7" — the latter records a **recurring AC-writing failure mode** (ACs that misdescribe reality fail correct implementations at UAT). **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog), **#1950** guard test recomputing the derived `Ѹ` ceiling (tech-debt, could have, Backlog, blocked-by #1939 — filed 2026-08-02 from PR #1948 §2; architect reframed its own ask from "re-run the 3,919-codepoint sweep" to "a test that recomputes 616 from `USAGE_WIDTH_7COL`/font sizes/`DEFAULT_LINE_HEIGHT`", sweep now an explicit non-goal. Rulings: **comment keeps the rationale, issue owns the guard, neither replaces the other**; bounded-and-quantified earns a tracked owner where unbounded-and-estimated gets documentation only; a derived bound with no test is a comment waiting to go stale — `overviewPdf.test.ts` pins `704`/`546` as literals tied to no geometry constant). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". **#1931 PR #1944 APPROVED round 1, 2026-08-02** — all ACs met **except 3.2/3.3, deliberately NOT claimed**: they assert live-model output quality, which a mocked LLM cannot verify. Ruling: **merge is a code gate, Done is an acceptance gate** — PR merges, story stays out of Done until a human reads real EN+DE output with `LLM_*` set; UAT scenarios posted on #1931; failure → reopen #1931, don't file a follow-up. Contrast #1909 AC 4.6: an unverifiable AC **with** a substitute assertion may be waived as a documented deviation; **without** one it goes to UAT. "Mit KI verbessern" accepted for AC 2.3. **#1917 L3 struck** (verified fixed); rest of #1917 open, **`KI` glossary entry still #1917's** — `glossary.json` untouched by #1944. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1931 reviewed". ## Requirements Coverage diff --git a/.claude/agent-memory/product-owner/bank-report-wizard.md b/.claude/agent-memory/product-owner/bank-report-wizard.md index 2f4fb3d3d..918413341 100644 --- a/.claude/agent-memory/product-owner/bank-report-wizard.md +++ b/.claude/agent-memory/product-owner/bank-report-wizard.md @@ -208,6 +208,20 @@ Final state worth knowing: table width is now **exactly 515.28pt, unfalsifiable - **#1940 — continuation rows read as broken** (`enhancement`, Could Have, Backlog). The deferred "Could Have" from the #1929 ruling, now *observed*: `splitIntoPageSafeChunks` has no minimum trailing-chunk floor, so a row can carry a **single stray character** with all other columns blank. Only above the chunk ceilings, i.e. beyond AC12's guaranteed 600-char range; no data loss (I1 holds). - **#1941 — editable override fields have no length limit** (`enhancement`, Could Have, Backlog). Zero `maxLength` in `client/src/components/reports/` or `EditableField/`; `attachmentsNote` is a client-side override that never round-trips, `areaText` is aggregate-unbounded (N × 200). **No longer a correctness risk** — round 4 bounded the renderer at cell scope. Input-side gap only. +### #1950 — guard test for the derived `Ѹ` ceiling (filed 2026-08-02 from PR #1948 round-3 review) + +`tech-debt`, **Could Have**, Backlog, **blocked-by #1939**. Filed off the architect's PR #1948 approval comment ([5160266124](https://github.com/steilerDev/cornerstone/pull/1948#issuecomment-5160266124) §2), which **reframed its own earlier ask**: the deliverable is *not* re-running the 3,919-codepoint sweep, it's a **guard test that recomputes** the derived ceiling from `USAGE_WIDTH_7COL` / `TABLE_BODY_FONT_SIZE` / `TABLE_SMALL_FONT_SIZE` / `DEFAULT_LINE_HEIGHT`. Sweep left out as an explicit **non-goal**, not an optional AC — an "optional" AC isn't binary and makes the issue unfalsifiable. + +The risk being guarded: `MAX_SAFE_USAGE_CHUNK_CHARS` (650) is **34 chars / 3 lines / 33.6pt over** its *derived* `Ѹ` ceiling of 616 (`44 lines × 14 chars`). Accepted on **input reachability** (needs 650 unbroken chars of archaic Church Slavonic Uk in one Usage cell), and because a `Ѹ`-safe value must sit in `[600, 616]`, collapsing AC12's margin over its 600-char floor from 8.3% to ~2.7%. `MAX_SAFE_SMALL_CHUNK_CHARS` (450) is genuinely safe (11.2% under 507). **Not a request to change 650** — the architect is comfortable with the risk. + +Three durable rulings, all written into the issue rather than left implicit: + +- **Comment and issue both, never one instead of the other.** The rationale stays in the code comment (AC 2.1 forbids moving/shortening/replacing it; AC 2.3 pins 650/450/1.04 and every width byte-identical) because *"anyone changing 650 or a column width reads that comment, not an issue tracker. Moving it out recreates the provenance loss that produced #1939."* The issue owns the **guard**; the comment owns the **rationale**. +- **Bounded-quantified vs unbounded-estimated is the line for "does this deserve a tracked owner."** `markerText` is unbounded with an estimated break-even → documentation only (folded into #1939). This is a bounded constant *provably* 34 chars past a derived ceiling → *"a quantified exceedance is a standing accepted risk with a number on it."* I would have collapsed these two; don't. +- **A derived bound with no test is a comment waiting to go stale.** Verified live: `overviewPdf.test.ts` pins `MEASURED_TRUE_CEILING` as re-typed `704`/`546` literals referencing **no geometry constant**, so widening the Usage column leaves them green while the real ceiling moves. Generalise: when a review accepts a *derived* number, ask what fails if its inputs change. + +AC 1.3 fails in **both** directions (growth widens a reviewed risk; shrinkage makes the comment's figure wrong). AC 1.6 keeps the measured 44/39-line budgets as the sole pinned literals, labelled as real-render measurements. The architect's two "informational, do not re-round" cosmetics (`~2.6%`→`~2.7%`, the self-asserted-infallibility sentence) were **already fixed at head `a6871975`** — checked before deciding, nothing folded in. + ### Merge/scope decisions in this triage - **`markerText` (unbounded, ~250-skipped-doc break-even) and `invoiceNumber` (unbroken, capped at 100) were folded into #1939 as a documentation-only AC**, not filed separately. Their value is entirely "the next person reading this file knows the enumeration"— the same category as the comment-scoping work, and a standalone Could Have would never be picked up. AC7 + a scope guard forbid actually implementing a bound for them. @@ -283,3 +297,49 @@ Reworded to "every viewport" with the 44×44px target **unconditional** (strictl - Before writing a viewport- or layout-conditional AC, **check the CSS for an actual breakpoint** — don't infer a responsive variant from the presence of `mobileCard` classes elsewhere in the same file. - For equivalence/"identical to" ACs, **name what is excluded** or sticky user preferences will read as failures. - When an AC is corrected on an open issue, annotate the AC inline with a date + pointer to the correction comment, and say in the comment *why*, so the original premise isn't reintroduced from memory of the old text. + +## #1932 user scope ruling 2026-08-02 — plain text with line breaks, not markdown + +Comments: `issuecomment-5160251632` (decision), `issuecomment-5160258752` (AC change log). + +**User, verbatim: "no full wysiwyg necessary - just a simple text body with line breaks".** So: plain text, no markdown, no rich-text editor, **no new client dependency**. Bold phrases and bulleted lists are **out of scope** even though #1932's Problem section cites them as motivation. The issue's "Needs a design decision before implementation" note is struck — this is no longer an architect/UX call. `ux-designer` still owns letter layout (AC 4.4). + +### Premise correction — the stated defect was largely false + +**pdfmake already honours `\n`.** `node_modules/pdfmake/js/TextBreaker.js` L30–34 and L53–58 treat `\n`/`\r\n` as a *required* line end, so a single text node renders embedded newlines as line breaks. The sender block has depended on this all along (`senderLines.join('\n')` in one node, pinned in `coverLetterPdf.test.ts` L80–90). The body's line-break round trip **already works and is merely unpinned**. + +Consequence: #1932 section 1 collapsed from a feature build to **regression guards + one new requirement**. Worth keeping the guard anyway — the per-token inline-run technique used elsewhere in `reportPdf/` for pdfmake's all-or-nothing `wordBreak` would silently destroy `\n` handling if ever applied to the body. That is a working-but-unpinned behaviour with a plausible silent breaker, which is exactly what a test is for. + +**Lesson: verify a "does not survive rendering" claim against the renderer's source before writing ACs around fixing it.** Cheap (one grep in `node_modules`), and it flipped this section's size. + +### The plain-text ruling *created* one requirement rather than removing it + +**AC 1.6 is now load-bearing.** `server/src/services/budgetExtraction/prompts.ts` L~142 ("Letter body") says nothing about output format. An LLM asked for a business letter readily emits `**emphasis**` and `- bullets`; under plain-text rendering those print as literal asterisks in the PDF a bank reads. Same defect class as the #1916 prompt-input findings — **when a formatting model is simplified, re-check what the LLM prompt assumes about it.** + +### Vacuous ACs: strike with a stated replacement, never delete + +- **1.4** (formatting discoverability) → STRUCK, replaced inline by a negative (must not advertise formatting support) + a concrete ask (`rows={6}` textarea must show multi-paragraph structure without scrolling). +- **1.5** (XSS-safe formatted rendering) → VACUOUS BY CONSTRUCTION, retained as a negative constraint: no `dangerouslySetInnerHTML`, no HTML parsing, no markup interpretation on the body path. Flagged as blocking if violated — it is precisely the AC a future "let's just add marked" PR breaks. +- **1.3** inverted: markup-looking characters must render **literally**; no markdown/rich-text dep added to `client/package.json`. + +Deleting a vacuous AC reads as an oversight and invites re-litigation; striking it with the replacement stated inline does not. + +### Non-formatting defects are genuinely independent — with one intra-issue coupling I had to add + +Sections 2 (signature), 3 (sender), 4 (layout), 5 (reset-X CSS) do **not** depend on the formatting model. Two caveats found on disk: + +- **§2 ↔ §3 are coupled to each other**, in code today: `applyOverrides.ts` L66–68 recomputes `signature` from an overridden sender (`sender.split('\n')[0]`), `types.ts` L44 documents `signature` as `DERIVED`, and `realRender.test.ts` L997 pins the recompute. Making signature first-class means a sender edit must stop overwriting an explicit signature, and that test must be **updated, not deleted**. Filed as new **AC 2.6** — would otherwise have been a review-time surprise. +- **Paragraph *spacing* moved §1 → §4.** With no markup carrying paragraph semantics, whether a blank line stays a full empty line or becomes typographic spacing is a layout call. AC 4.1 amended to own it. +- §5 (reset-X) is pure shared-component CSS and is fully severable — could be split out if #1932 ever needs shrinking. + +### #1925 fold-in unaffected + +All four carried ACs stand verbatim — #1925 is the date **caption** in the letter head plus caption chrome styling, neither of which touches the body model. Added a note above section 6 saying so, so the ruling isn't read as having disturbed the fold-in. + +### #1939 → #1932 handoff (blocked-by is a single edge) + +`#1932 blockedBy` = **only #1939** (verified via GraphQL `blockedBy(first:10)`). PR **#1948** open, no review posted yet. Clear the edge as soon as #1948 merges; nothing else gates #1932. Post-#1939 facts recorded in #1932's Notes for the implementer: + +- **`PDF_STYLES` is defined in `pageGeometry.ts`** and re-exported from `merge.ts` (existing `from './merge.js'` imports keep working) — new letter styles go in `pageGeometry.ts`, which must **never** import from `merge.ts`. +- **`HEADER_ROW_HEIGHT_MAX`** is a ceiling (68pt vs 45.81pt measured), not an estimate. The cover letter ends with a hard `pageBreak: 'after'` so it never shares a page with the table — if #1932's implementation reaches for the constant at all, the layout approach has drifted. +- #1932 should not touch `overviewPdf.ts`; #1937 owns that file's German-header work. diff --git a/.claude/agent-memory/translator/MEMORY.md b/.claude/agent-memory/translator/MEMORY.md index 2f4623b28..5c74d3d50 100644 --- a/.claude/agent-memory/translator/MEMORY.md +++ b/.claude/agent-memory/translator/MEMORY.md @@ -100,3 +100,14 @@ New `sourceReports.expand.*` (chevron-expand sub-tables for budget lines + depos ## Audit Protocol History - [Audit pitfalls](audit-pitfalls.md) — incident history behind the mandatory 4-step full-coverage audit protocol: a parity-only audit missed 13 code-referenced keys (Area UI raw-key bug); loose substring greps flagged 52 false positives + +## Cover Letter Signature Block Keys (Issue #1932, 2026-08-02) + +`sourceReports.editable.signatureLabel` → "Unterschrift"; `sourceReports.coverLetter.closing` → "Mit freundlichen Grüßen,"; `sourceReports.editable.closingLabel` → "Grußformel". Confirmed: neither "signature" nor "closing salutation" belongs in the glossary (grep across `glossary.json` for signature/closing/Gruß terms found nothing, and these are generic letter-writing vocabulary, not Cornerstone domain terms) — did not add. + +**Chrome-vs-content split, worked example**: `coverLetter.closing` and `editable.closingLabel` sit right next to each other in the UI but are translated by two different rules, and getting this backwards is the easy mistake: +- `coverLetter.closing` = the letter's actual salutation text, rendered via `reportT()` into the PDF in the *report's* language → full letter-register translation ("Mit freundlichen Grüßen,"). +- `editable.closingLabel` = the read-only caption *above* that value in the step-5 editor, rendered via `t()` in the *interface's* language → UI chrome, translated like its sibling captions (`senderLabel`→"Absender", `subjectLabel`→"Betreff"), using the precise German business-letter term for "closing salutation" ("Grußformel", paired with "Anrede" for the opening salutation) rather than a literal "Schließen"/"Closing"-style calque. +- Rule of thumb: check the **call site**, not the JSON namespace path — both keys live under `sourceReports.*` but `t()` vs `reportT()` at the usage site is what actually decides chrome vs. content, per the established artifact-content-vs-edit-affordance convention from #1909/#1924. + +**Concurrent-worktree gotcha**: at spec time `closingLabel` didn't exist in `en/budget.json` yet (frontend-developer added it mid-session, confirmed via `git diff client/src/i18n/en/budget.json` showing the line appear between my two check-ins). When a spec says "check whether the en key already landed," re-check immediately before finalizing rather than trusting an earlier read — in a shared worktree the answer can flip within the same task. diff --git a/.claude/agent-memory/ux-designer/MEMORY.md b/.claude/agent-memory/ux-designer/MEMORY.md index 2ed7884fa..6e39ec0a1 100644 --- a/.claude/agent-memory/ux-designer/MEMORY.md +++ b/.claude/agent-memory/ux-designer/MEMORY.md @@ -29,4 +29,6 @@ - `formatDateForAria`-style aria-labels that interpolate localized weekday/month into a hardcoded English sentence order (`"{weekday}, {month} {day}, {year}"`) render grammatically odd in German (`"Dienstag, Februar 24, 2026"` vs correct `"Dienstag, 24. Februar 2026"`) — this is an accepted, deferred, non-blocking gap across the app, not a new bug to flag each time. - When a PR claims a Badge/shared-component consolidation is "zero visual delta," build a before/after table (padding/font-size/weight) per migrated component — the shared base class is usually extracted from only a subset of sources, so other migrated components can silently inherit a different size than they had. See PR #1848 in [pr-review-findings.md](pr-review-findings.md). - For `client/src/lib/reportPdf/` PRs (pdfmake document generation, no CSS/components/dark-mode in scope): don't judge column widths/wrapping/page-break behavior from font-metric arithmetic — render the real pipeline to a throwaway `/tmp/*.pdf` via a scratch Jest test and rasterize with `pdftoppm` for visual inspection, then delete the scratch file. See [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md) for the exact technique and a confirmed `dontBreakRows` row-split bug found this way. +- pdfmake: an empty-string text node (`{ text: '' }`) reserves the SAME line-height as a non-empty node — never worry it'll collapse to zero, no nbsp workaround needed for an "always reserve this line's space" AC. Also: at 11pt/1.4 line-height, a real rendered line is ~18pt, not the naive `fontSize*lineHeight=15.4pt` — render-and-measure, don't hand-compute. See [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md), Issue #1932. +- When an AC complains a UI panel's caption/label "mixes languages" with its value, check whether the real bug is inline concatenation (`"Label: value"` on one line) before translating the caption into the value's language — if sibling captions in the same panel are all interface-language chrome, translating just one caption breaks panel consistency instead of fixing it. Fix by visually separating caption from value (label-above-value, matching the panel's existing label recipe) instead. See Issue #1932 in [feature-spec-history.md](feature-spec-history.md). - **`:global(.foo)` in a `.module.css` file only matches a DOM element whose class is the literal unhashed string `"foo"`.** If a component applies another module's class via plain `className={otherModuleStyles.foo}` (not `composes:`), that class resolves to a hashed string (`foo_a1b2c` per `webpack.config.cjs`'s `localIdentName`) in real builds — `:global(.foo)` selectors written to "layer styles on top of" that class never match, silently killing the entire rule block. This is invisible in Jest: `jest.config.ts` maps `.module.css` → `identity-obj-proxy`, which resolves classes to their literal key name, so `:global(.foo)` _does_ match in tests. The correct cross-module extension technique is `composes: foo from '../other/Other.module.css';` inside a locally-scoped class, then apply _that_ local class — this actually merges the real hashed class list onto the element. Check for this specific pattern (`:global(...)` selectors targeting another module's class name, applied via plain className not `composes`) whenever a new component's CSS "doesn't seem to apply" or a spec's described states (hover/focus/at-rest tint/indicator dots) go missing — see PR #1909 in [pr-review-findings.md](pr-review-findings.md). diff --git a/.claude/agent-memory/ux-designer/feature-spec-history.md b/.claude/agent-memory/ux-designer/feature-spec-history.md index 7fbaec1a2..0c15c1f80 100644 --- a/.claude/agent-memory/ux-designer/feature-spec-history.md +++ b/.claude/agent-memory/ux-designer/feature-spec-history.md @@ -157,3 +157,12 @@ Part of the Bank Report Wizard Refinement Round 2 mini-epic (follows #1898). Spe - **Focus ring decision**: deliberately did NOT add a custom `box-shadow: var(--shadow-focus)` ring to the new radio group — kept the native browser `:focus-visible` outline to stay consistent with the sibling `.optionCheckbox` controls in the very same step, which also rely on the native outline (no custom ring exists there today). Don't treat "every new control must get a custom focus ring" as an absolute rule — matching the immediate visual siblings can be the more consistent choice. - **Found a real responsive risk while auditing `WizardStepper` for a 5th step**: the desktop `