diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index dcd665f33..c2037cf37 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -29,6 +29,7 @@ - [story-1901-ai-report-generation.md](story-1901-ai-report-generation.md) — AI-generated usage descriptions/cover letter: new `reportWizardAiGeneration.spec.ts` + POM AI locators; llmEnabled-mock pattern; overwrite-modal-guard-checks-overrides-not-aiContent gotcha; cover-letter-fields-need-contactAddress/reference seed trap; reused auto-itemize LLM error strings. - [story-1900-editable-report-preview.md](story-1900-editable-report-preview.md) — step 5 reworked from always-present auto-regenerating iframe → live editable surface + on-demand PDF Modal; POM rename `waitForPreviewReady/Regenerated` → `openPdfPreviewModal`/`closePdfPreviewModal` (must close before another modal-opening action); 4 filed bugs (#1904-#1907, all now FIXED+CLOSED, see file's re-verification section); deliberate deviation editing a file explicitly marked "do not touch" because leaving it broke `tsc` workspace-wide — see file for the general lesson; `footnoteFetchFailed` skip note naturally reachable with no Paperless container, no mock needed; re-verification added `mobileCard()`/`mobileUsageField()` POM locators + Scenario 15 (#1907 regression guard) + found/filed NEW bug #1908 (mobile-card fallback visible on desktop, no default `display:none`) with its own expected-to-fail Scenario 1b. +- [issue-1959-inline-meta-and-labels.md](issue-1959-inline-meta-and-labels.md) — #1959 reversed #1923's †/‡ footnotes → inline `(partial)`/`(less deposit)` and merged area+attachments into one `.usageMetaText` line; POM renames, rewritten scenarios 2/17/18/20 + AI 8, attachment-tier facts, untested column toggles. - [claim-deposit-scope-1922.md](claim-deposit-scope-1922.md) — PR #1922 invoice/deposit claim-scope split: `handleMarkClaimed`'s two-array submit, server-truth success-banner counts, the three "deposit surfaces the invoice" shapes, `claimNothingClaimable` guard. ## Open follow-ups to flag to orchestrator diff --git a/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md b/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md new file mode 100644 index 000000000..5375285b2 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md @@ -0,0 +1,55 @@ +--- +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. +metadata: + type: project +--- + +PR #1959 ("improve report PDF UX") deliberately **superseded** two designs earlier rounds had +asked for, in `ReportContentEditor.tsx` / `buildReportContent.ts`: + +1. `†`/`‡` markers + the shared footnote list (Story #1923 AC1) → grey inline `` in the **Allocated Amount cell**: `(partial)` / `(less deposit)` + (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 + **negative-only** guards. +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 + entirely, so a content-table row / mobile card now has **exactly one textbox** (Usage) — the + crispest available guard against that column coming back. + +**Why:** the user owns #1959 and asked for it in the promotion; source-of-truth hierarchy makes +the PR body the spec, so the tests were rewritten, not the code. + +**How to apply:** POM renames are `usageAreaText`→`usageMetaText`, +`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). + +Facts worth reusing: + +- `toHaveText`/`toContainText` normalize whitespace, so the desktop `inlineNote` span's leading + space (` (partial)`) is absorbed — `toHaveText('(partial)')` is correct. +- Attachment note text for a `claim` report with `attachmentType: 'invoice'` is + `1 attachment: Invoice`. The tier gate is + `server/src/services/shared/attachmentTierUtils.ts`: floors are quotation(1) for + budget-overview, deposit(2) for claim, invoice(3) for proof-of-funds; `null` counts as tier 3. + So a 'invoice'-tagged link shows up in claim reports — don't guess, that file is the only + definition. +- `document_links` is unique on `(entity_type, entity_id, paperless_document_id)`, so a + hardcoded `paperlessDocumentId` in a spec cannot collide across parallel workers/projects. +- `AppShell.tsx` renders a real `
` element, so `page.locator('main')` is a safe + page-scope text container. +- **Column-visibility checkboxes (`role="group"`, "Show/hide columns") shipped with #1959 with + NO E2E coverage** — deliberately not added on the critical path, because a new test case + reshuffles shard membership. Pick this up when the promotion isn't blocking. +- Sub-agents share the worktree here: a `prettier --check` on a `client/` file can transiently + fail because another agent is mid-write. Re-check before reporting it as broken. + +See [[known-flakes-and-regressions]], [[story-1900-editable-report-preview]], +[[story-1879-report-wizard]]. diff --git a/.claude/agent-memory/frontend-developer/MEMORY.md b/.claude/agent-memory/frontend-developer/MEMORY.md index f8666cdbf..ecb0a2816 100644 --- a/.claude/agent-memory/frontend-developer/MEMORY.md +++ b/.claude/agent-memory/frontend-developer/MEMORY.md @@ -98,6 +98,8 @@ removing it will fail typecheck. Keep the export and note it as QA-deferred. - `budget-invoices.md` — open for budget/invoice/subsidy pages: BudgetSubNav, budget line APIs, invoice CRUD patterns, pipeline dashboard cards - `gantt-calendar.md` — open for Timeline/Gantt/Calendar/Milestone work: SVG architecture, drag hook (React 19 refs rule), arrow hover, lane allocation, constraints section, related E2E POM layout - `api-routing.md` — open when calling APIs or navigating: UserResponse/PaginatedResponse shapes, auth routes, dependency direction-swap semantics, navigation origin state +- `i18n-invisible-chars.md` — open before adding an NBSP/invisible char to a locale file: literal-not-escape convention, why grep misses it, how to verify by render, and the identical-looking test failures it causes +- `qa-tripwire-tests.md` — open when a task says a fix will flip an `it.failing` guard: why the flip signal can be swallowed by a shared helper, and what to do instead - `testing-mocks.md` — open when your change will break existing tests: ESM mock patterns, mock-block update checklist, Toast/context mocks, DefinePlugin globals in Jest - `git-tooling.md` — open on webpack config changes or git/CI trouble: worktree remotes, object corruption/permission recovery, npm audit policy, Prettier/CI format gap - `photo-metadata-sidepanel.md` — photo metadata side panel notes (open for photo UI work) diff --git a/.claude/agent-memory/frontend-developer/i18n-invisible-chars.md b/.claude/agent-memory/frontend-developer/i18n-invisible-chars.md new file mode 100644 index 000000000..a1200de3d --- /dev/null +++ b/.claude/agent-memory/frontend-developer/i18n-invisible-chars.md @@ -0,0 +1,34 @@ +--- +name: i18n-invisible-chars +description: How to add and verify invisible characters (NBSP) in locale files — repo writes literal non-ASCII, and the resulting test/diff failures look like identical strings +metadata: + type: project +--- + +Multi-word inline labels rendered into the PDF wrap at their internal space. A single-word label +can't wrap; a two-word one breaks across lines and splits its own brackets (`(less` / `deposit)`). +The fix is U+00A0 (NBSP) instead of the space — same glyph advance, so no geometry constant moves, +and pdfmake's UAX-14 line breaker treats it as non-breaking. + +**Why the details matter:** the change is invisible in every textual channel, so it produces +failures and diffs that look like no-ops and burn reviewer time. + +**How to apply:** + +- **Convention:** `client/src/i18n/**` uses **literal** non-ASCII characters (`…`, `—`, `·`, `↑`) — + there are zero `\uXXXX` escapes. Write a literal NBSP, not an escape. Insert it with a script + (`python3` + `u' '`), never by typing it, and verify with + `python3 -c "...print([hex(ord(c)) for c in value])"` — a hand-typed one silently lands as a plain + space. +- **grep will not find it:** `grep -P '\xc2\xa0'` returns nothing in a UTF-8 locale (PCRE reads the + escapes as characters, not bytes). Use `LC_ALL=C grep -n $'\xc2\xa0' `, or `git diff | cat -A` + (NBSP shows as `M-BM-`). +- **`pdftotext` normalizes it back to a space**, so extraction can't confirm the character. Verify + the **break position** instead: render the real PDF, extract with `pdftotext -layout`, and check + the label lands on one line. Render a plain-space control alongside it — otherwise you can't tell + a fix from data that happened not to wrap. +- **Warn about the test fallout explicitly.** Any test asserting the label literally now fails with + `Expected " (less deposit)" / Received " (less deposit)"` — visually identical. Always report the + codepoints and the exact replacement, or someone will read it as a flake. + +Related: [[qa-tripwire-tests]]. diff --git a/.claude/agent-memory/frontend-developer/qa-tripwire-tests.md b/.claude/agent-memory/frontend-developer/qa-tripwire-tests.md new file mode 100644 index 000000000..631af7199 --- /dev/null +++ b/.claude/agent-memory/frontend-developer/qa-tripwire-tests.md @@ -0,0 +1,35 @@ +--- +name: qa-tripwire-tests +description: QA encodes known production defects as `it.failing` tripwires; a shared helper that hard-codes the broken model can swallow the flip signal, so verify the fix independently +metadata: + type: project +--- + +QA (`qa-integration-tester`) encodes a known-but-unfixed production defect as `it.failing` with a +passing control test alongside. The suite stays green while the defect is open; when production is +fixed the test starts passing and Jest errors with "Failing test passed even though it was supposed +to fail" — that error is the intended confirmation signal, and the fixing agent is expected to flip +`it.failing` → `it` (the one sanctioned developer edit to a test file). + +**Why:** it keeps the defect encoded in executable code rather than only in a report, so a fix can't +land without someone noticing the guard. + +**How to apply:** the flip signal is NOT reliable on its own. If the tripwire calls a shared render +helper that asserts the _broken_ model (e.g. `expect(body).toHaveLength(1 + usageChunkCount + 1)`, +which bakes in "meta never adds rows"), a correct fix makes the helper throw first — the tripwire +then still "passes" as a failing test and no signal appears. Seen on #1959 (`realRender.test.ts` +cell-scope block): the fix was verified, the tripwire never flipped. + +So: (1) always verify the fix independently with your own scratch render/measurement harness before +trusting or distrusting the tripwire; (2) if the flip leaves the test failing inside a shared helper +rather than at its own assertions, revert to `it.failing`, leave QA's file byte-identical, and report +the exact helper line QA must update — do not "fix" the helper to make the flip work; +(3) expect sibling tests in the same block to fail too, for the same reason (they assert the broken +model at content-tree level, which is why they passed while the rendered PDF lost content). + +**Outcome on #1959 (closing the loop):** reporting the exact blocking helper line was the right call +— QA re-derived the row count from the production packer itself, then converted the tripwire to a +normal guard. Reporting "cannot flip, here is the line" got it resolved in one round; editing the +helper myself would have put a developer edit in a QA-owned file for no gain. + +Related: [[../MEMORY.md]] "Refinement Workflow — QA Test Coordination", [[i18n-invisible-chars]]. diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index c966f33e6..373f93876 100644 --- a/.claude/agent-memory/product-architect/MEMORY.md +++ b/.claude/agent-memory/product-architect/MEMORY.md @@ -2,11 +2,11 @@ ## Topic Files -- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955) +- [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) - [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 + the reportContent content/layout split (#1900) +- [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) - [Diary drafts pattern](diary-drafts-pattern.md) — ADR-022 draft lifecycle via status column on parent table - [EPIC-03 refinement](epic03-refinement.md) — 40 consolidated refinement items - [EPIC-04 household items](epic04-household-items.md) · [EPIC-05 budget](epic05-budget.md) · [EPIC-17 i18n](epic17-i18n.md) · [EPIC-18 areas & trades](epic18-areas-trades.md) diff --git a/.claude/agent-memory/product-architect/client-pdf-pipeline.md b/.claude/agent-memory/product-architect/client-pdf-pipeline.md index 4b9ee56de..746bccc37 100644 --- a/.claude/agent-memory/product-architect/client-pdf-pipeline.md +++ b/.claude/agent-memory/product-architect/client-pdf-pipeline.md @@ -215,3 +215,49 @@ baseline rather than blanking a field. Keep this shape — putting AI output in Server side: `POST /api/source-reports/generate-content` re-fetches the report (client sends selection only), persists nothing, and reuses the single `budgetExtraction/` LLM gateway as a third provider method. Both documented in ADR-034 "Addendum: content layers". + +### PR #1959 review — the Usage-cell height bound, third scope revision + +**The recurring defect** (#1929 r3 -> r4 -> #1959 r1) is always the same: `table.dontBreakRows: true` makes +pdfmake measure an over-tall row and then `PageElementWriter` **silently discards** the overflow. No throw, +no visible truncation. Symptom: page count **saturates** (~2) and can go **non-monotonic** (3 -> 2 as content +grows) while rendered line count rises linearly. + +**The rule that fixes it durably: bound the height of what a cell RENDERS, never of a source field.** +Any per-field bound is coupled to a layout decision and detaches silently the moment the layout changes — +which is exactly how r4's per-field continuation rows became #1959's unbounded inline run. +`packUsageCellRows(segments, MAX_SAFE_USAGE_CHUNK_CHARS)` is the right shape because the bound now lives +where the cell's content **stream** is assembled, so a new segment is bounded automatically. + +**Two detection recipes that actually caught it** (both threshold-free, both worth reusing for any generated +document): page count must be **monotonic** in content size, and **channel-independent** — the same text +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 +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. + +**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 +affect the PDF widens the Usage column and **invalidates the measured ceiling** — "just plumb the toggles +through" is a re-measurement story, not a UI change. #1959's toggles are preview-only local `useState`. + +**`packUsageCellRows` hangs on `maxChars <= 0`** (own `remaining <= 0 -> flush() no-op -> continue` loop). +New to the packer — `splitIntoPageSafeChunks` fails loudly instead (`RangeError`). Unreachable while the +budget is a constant; matters if it ever becomes computed. + +### 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`. + 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. diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index 2016c09f5..65a674ab1 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -389,3 +389,32 @@ in flight. That is the whole proof. Two corollaries worth asserting in review: Ordering claims: a client queue genuinely fixes server-side out-of-order application only if the next write is not _dispatched_ until the previous response is back (i.e. `await` wraps the HTTP call, not just the local state update). Check that, or the fix only narrows the window. + +## Capability retained in code, disabled at the producer, documented by comment (PR #1959) + +Three instances in one PR, all the same shape — **a type permits a state the producer can never emit, and the +constraint is enforced by a comment instead of by the type**: + +- `ReportContent.footnotes` is now unconditionally `[]`, yet 4 consumers still branch on / copy it. The E2E + page object had to add a prose warning ("never assert a positive count on these"). +- `applyOverrides` still honours `row..attachmentsNote` with **no producer left** (editor field removed; + overrides are ephemeral wizard state — no server persistence, nothing in migrations 0001-0044; and + `applyAiContent` only writes `usageText` + cover-letter fields). +- Column-visibility toggles labelled `Show/hide columns` that affect the preview but **not** the exported PDF. + +Rule: **delete rather than comment.** A write path with no reader and no producer is how the #1929 +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. + +## Verify the AC record when a PR reverses a recently-shipped story (PR #1959) + +#1959 reversed #1923's AC1.1/1.2/2.3/2.4 **one day** after #1923 shipped (2.13.0-beta.38, 2026-08-02), while +#1923 stayed CLOSED with those ACs marked delivered — so the record asserted two contradictory behaviours were +both correct on `beta`, and the pending promotion PR would have summarized reversed ACs as delivered. + +Tell: the reversing issue had **zero comments**, no `**[product-owner]**` header, no numbered ACs (its body was +a PR-style summary), and no ux-designer visual spec — whereas the story it reversed had all four. **A +requirements reversal authored as a polish issue is the signature.** Cheap fix: PO supersession comment on the +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). diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index d6c6a9623..7ebea615a 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -41,6 +41,7 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) - **DataTable: #1955** two-column toggle race silently hides 2nd column, all 6 DataTable pages (Should Have, S, Backlog, 2026-08-02). See [datatable-column-preference-race.md](datatable-column-preference-race.md) — records that **fast clicking is the SAFE case** (I judged this backwards; debounce `clearTimeout` coalesces rapid input, the >500ms reading-pace gap is the reachable one) and that #1920's E2E-only fix (`InvoicesPage.enableColumn()` awaits the PATCH) makes CI green **without** fixing production — don't close #1955 on a green shard. - **#1957** latent cross-file E2E test-isolation hazard (shared-admin `user_preferences` writes under `fullyParallel`, `LocaleContext.syncWithServer` actively flips a victim test's locale) — Should Have, bug, Backlog, 2026-08-02/03, filed from `/fix-e2e` work on PR #1956. Scoped as an audit + per-spec sweep, not a single-file fix — found a second live instance (`diary-uat-fixes.spec.ts` vs `dashboard.spec.ts`, key `dashboard.hiddenCards`) while researching it. Distinct from #1955 (production race) and #1920 (E2E workaround for #1955). Detail in [e2e-shared-admin-preference-hazard.md](e2e-shared-admin-preference-hazard.md). - **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) — **PR #1951 APPROVED round 1, 2026-08-02**, all 40 ACs met incl. both struck-as-vacuous negative constraints; 3 MUST FIX (German `Mit freundlichen Grüßen` comma, unpinned Closing row, #1925 AC5 editor pin). **#1925 CLOSED as duplicate** (board Wont-Do) — my §6 carried 4 of its 6 ACs, a second instance of the AC-transcription failure mode, **#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". +- **PR #1959 (report PDF UX, user's own PR, held promotion #1958) — 3 rulings 2026-08-03.** Footnotes `†`/`‡` + their sentences replaced by inline labels `(partial)`/`(less deposit)`, reversing **#1923 AC1.1/1.2/2.3/2.4** (which had already superseded #1898 §4). **Ruled: labels kept, sentences return as a non-editable report-level legend → #1965 (Must Have)** — `(partial)` is near-self-evident beside the two amount columns, but `(less deposit)` drops "claimed **separately**", a materially different audit claim. Cheap because the footnote channel is orphaned-not-removed (producer-only fix). **Closed/released ACs get a dated supersession comment, never a rewrite** (posted on #1898 + #1923). #1923 AC5.3 substance survives — `areaText` still a separate field, so judged against its stated rationale, not its prescribed sub-line rendering. Glossary: **`Abschlag` APPROVED** as a measured-space short form of `Abschlagszahlung` (option (c) is arithmetically impossible — the full term eats 72.85 of 75pt, so no qualifier fits; collapsing into the constituted-deposit label would lose more); **`split`'s three German forms get NO entry** (glossary prevents semantic divergence, is not a surface-form registry). Both on **#1917**. Issues filed: **#1965** legend, **#1966** column-toggle E2E, **#1967** `attachmentsNote` override unreachable, **#1968** meta-suffix single run, **#1969** `testPrefix`/`authenticatedPage`, **#1970** configurable auth rate limits, **#1971** `search-users.spec.ts` leftovers, **#1972** silent failed column-pref saves + dead `isLoaded`. Full rulings in [bank-report-wizard.md](bank-report-wizard.md) §"PR #1959 rulings". ## 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 d7fd7ce0d..a0596a427 100644 --- a/.claude/agent-memory/product-owner/bank-report-wizard.md +++ b/.claude/agent-memory/product-owner/bank-report-wizard.md @@ -396,3 +396,50 @@ Architect's PR #1951 review ([comment 5160566459](https://github.com/steilerDev/ ### PR body staleness — checked downstream, clean The "No new E2E coverage — deferred" line was already corrected (now lists Scenarios 21-24, checkbox ticked). Checked both propagation paths: no open `beta → main` promotion PR, and `RELEASE_SUMMARY.md` predates the report-wizard work entirely (it covers Cost Basis filter / auto-itemize merge / diary defaults) and is regenerated by `docs-writer` at promotion. Nothing downstream consumed the stale claim. + +## PR #1959 rulings (2026-08-03) — footnotes removed, glossary short-form approved + +`fix(reports): improve report PDF UX`, the **user's own PR**, held the `beta → main` promotion (#1958). Three product questions, all ruled without blocking promotion. + +### The reversal: shared footnotes → inline labels + +#1959 removed the `†`/`‡` markers **and their explanatory sentences**, replacing them with inline grey labels on the allocated amount: `(partial)`/`(Teilbetrag)` and `(less deposit)`/`(abzgl. Abschlag)`. This reverses **#1923 AC1.1, AC1.2, AC2.3, AC2.4** — which #1898 §4 had itself already been superseded by. `allocatedMarkers` gone from `types.ts`, replaced by `isSplit`/`isDepositReduced`. + +**Ruling: labels accepted, sentences must come back as a report-level legend (#1965, Must Have).** The differentiator is *which* sentence is load-bearing: + +- `(partial)` is nearly self-evident — the table prints **Invoice Amount** and **Allocated Amount** side by side, so the label only has to name the reason for a difference the reader can already see. +- `(less deposit)` loses a **materially different claim**. The footnote said the deposit was claimed **separately** — accounted for in another submission. "less deposit" is equally consistent with the deposit never being claimed, and leaves a double-claim question open when it resurfaces. On a Verwendungsnachweis/Mittelabruf that has audit consequences. + +**Generalizable: for outbound financial copy, ask what a label lets the reader *conclude*, not whether it is accurate. `(less deposit)` is true and still misleading by omission.** + +**Why the fix is cheap (checked, load-bearing for the ruling):** the footnote *rendering channel is fully intact* and merely unused. `buildReportContent.ts` declares `footnotes: ReportContentFootnote[] = []` (L232) and returns it (L289) but never pushes; `ReportContentEditor.tsx` L445-450 renders it; `overviewPdf.ts` appends `reportContent.footnotes` **verbatim** (pinned by the `overviewPdf.test.ts` case "appends reportContent.footnotes verbatim…", which is also the only surviving reader of the two i18n keys — as fixture text). So the legend is a **producer-only change** and preview/PDF parity comes free. **Pattern: before ruling a restoration too expensive, check whether the mechanism was removed or only orphaned.** + +**Legend must NOT go in the cover letter** — it is user-editable (#1932), so an editable qualification can be deleted, silently removing a material statement from a financial document. Report-level and non-editable, like the existing footnote block. + +**The orphan-key cleanup was deliberately NOT filed as a deletion issue.** `splitFootnote`/`depositReducedFootnote` are *consumed* by #1965, so filing "delete these orphans" would race the legend. #1965 AC 3.2 pins retention; its Notes record deletion as the Wont-Do alternative. **Pattern: when a cleanup follow-up and a restoration follow-up target the same artifact, one issue must own both or the cleanup wins by arriving first.** + +### What was NOT an AC reversal (checked, initially framed as one) + +**#1923 AC5.3 substance survives.** The area name moved from a sub-line to an inline grey suffix, but `areaText` is **still a separate row field** (`buildReportContent.ts` L196/L214) and `applyAiContent.ts` still only assigns `row.usageText` (L50) — so AI-generated usage text cannot drop the area, which was AC5.3's *stated rationale*. #1923's own Notes delegated the area sub-line's **visual treatment** to `ux-designer`. So E2E Scenario 20's rewrite is a presentation change within delegated authority. **Pattern: an AC that states its own rationale should be judged against the rationale, not the prescribed rendering — that is what the rationale is there for.** + +### Stale ACs on closed/released issues — comment, don't rewrite + +Both #1898 and #1923 are CLOSED + `released on @beta`. **Ruled: do not rewrite their ACs; post a dated supersession comment** naming which ACs died, by which PR, what still holds, and where the new source of truth is. Rewriting a released story's ACs falsifies the record of what was accepted and loses the design reasoning. The standing "stale AC is a real defect" rule targets ACs that a *future* implementer or UAT run will read as live spec — a supersession comment discharges that risk without destroying history. Posted on both (#1923 comment also lists the non-superseded ACs explicitly, since AC2.1/2.2/2.5/3/4 all still hold). + +Also noted on #1898: its §4 has now been superseded **twice** (#1898 → #1923 → #1959). That churn is itself the argument that the footnote *presentation* was never settled, and it supports abandoning the glyphs — which is why #1965 keeps the sentences but not the markers. + +### Glossary: `Abschlag` APPROVED as a space-constrained short form + +Full ruling on **#1917** (the pending glossary-refinement pass, same place the `KI` entry lives; translator implements, PO does not edit `glossary.json`). + +`depositReducedInlineLabel` = `abzgl. Abschlag` ships as written, against the glossary-approved `Deposit → Abschlagszahlung`. Translator measured with fontkit against the real embedded `Roboto-Regular.ttf` at 8pt in the fixed 75pt `ALLOCATED_AMOUNT_WIDTH`: `" (Abschlagszahlung)"` = 72.85pt (the existing sibling badge, so it **sets the real ceiling**), `" (abzgl. Abschlag)"` = 63.95pt, `" (abzgl. Abschlagszahlung)"` = **96.07pt, overflows by ~21pt**. + +**The decisive arithmetic the options list missed: option (c), "a shorter compliant string", is unavailable.** `Abschlagszahlung` alone eats 72.85 of 75pt, so **no qualifier of any length fits** — not `abzgl.`, not `ohne`, not the accounting `./.`. Keeping the full term therefore forces dropping the qualifier, collapsing this label into the *constituted*-deposit label (`(Abschlagszahlung)`, #1923 AC2.1). Those are different facts — "this row **is** a deposit" vs "this amount is **reduced because** deposits were claimed separately". **Collapsing them is a worse information loss than the abbreviation.** + +Option (b), widening the column, rejected on **risk not cost**: `ALLOCATED_AMOUNT_WIDTH` is #1929 geometry that took **four rounds** and real render-and-rasterize measurement, precisely because Usage was collapsing and the table overflowed the page edge. Reopening it for zero reader benefit is a bad trade. And `Abschlag`/`Abschlagszahlung` are the same concept in German construction practice (cf. `Abschlagsrechnung`); `abzgl. Abschlag` is idiomatic invoice German. No reader is misled — the bar set by the Verwendungsnachweis/Einreichung precedent. + +**Why it earns a glossary entry rather than a silent exception: without one a future compliance sweep "fixes" it in good faith and silently breaks a bank-facing PDF, invisibly to the unit suite.** The entry must record the 75pt column and the measurement as the *reason*, not just list the variant. **Generalizable: a deliberate deviation from an approved term needs a recorded reason at the glossary, or the next sweep reverts it.** + +### Glossary: `split`'s three German forms — NO entry, deliberately + +`anteilig` (adj.) / `Anteil` (noun) / `Teilbetrag` (noun), each role-correct, mirroring English's own `partial`/`split`/`portion`. **Ruled: not drift, no entry.** The glossary prevents *semantic* divergence — one concept becoming two concepts. It is **not a single-surface-form registry**, and pinning one form here would force ungrammatical copy across an adjective and two nouns. Recorded on #1917 as "reject if proposed later" so a future translator does not re-escalate. Revisit only if the *English* is unified — a copy story, not a glossary one. diff --git a/.claude/agent-memory/qa-integration-tester/MEMORY.md b/.claude/agent-memory/qa-integration-tester/MEMORY.md index a42ecf663..633f8b800 100644 --- a/.claude/agent-memory/qa-integration-tester/MEMORY.md +++ b/.claude/agent-memory/qa-integration-tester/MEMORY.md @@ -15,6 +15,7 @@ ## Recent bug/story notes (2026-08) +- [PR #1959 — inline meta content loss + `it.failing` tripwires](pr-1959-inline-meta-content-loss.md) (2026-08-03, RESOLVED) — prod defect found+fixed (unchunked meta in a `dontBreakRows` cell silently drops pages); **a tripwire is worthless if a shared helper bakes in the buggy assumption** — mine nearly stayed green through the fix; channel-equivalence is the threshold-free assertion; tree-level assertions cannot see this bug class; **write NBSP as `\u00A0` in test expectations, never a literal** (I smuggled one into the guard against it); keep literal+invariant at different levels; non-positive chunk budgets HANG not throw; `grep` silently returns nothing on these test files (use `awk`). - [Bug #1955 — echo-race harness + mutation probes](bug-1955-echo-race-harness.md) (2026-08-03) — echo must fire on the write's *resolve* (not the call) or the queue fix masks the guard and the test passes pre-fix; `rerender()` stands in for the optimistic `setPreferences`; 4 perl mutation probes prove each test guards a distinct part of the fix; never run repo-wide `npm run format` (38 unrelated files drift). - [Story #1930 — attachment tier rules](story-1930-attachment-tier.md) (2026-08-02) — new `attachmentTierUtils.test.ts` (100% cov, table+explicit-null pattern); sourceReportService.test.ts 16a/b/c/e → AC1/AC2/AC3/AC5; cross-report-type test needs a fresh invoice per report type; round 2 added a genuinely discriminating proof-of-funds fixture (deposit-only invoice, tagged+claimed deposit) after PO flagged the original AC3/16 pof blocks weren't change-detecting — inline-swap isolation technique to prove it fails on old logic. diff --git a/.claude/agent-memory/qa-integration-tester/pr-1959-inline-meta-content-loss.md b/.claude/agent-memory/qa-integration-tester/pr-1959-inline-meta-content-loss.md new file mode 100644 index 000000000..012efd02d --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/pr-1959-inline-meta-content-loss.md @@ -0,0 +1,176 @@ +--- +name: pr-1959-inline-meta-content-loss +description: PR #1959 (report PDF UX) reintroduced and then fixed the #1929 round-4 PDF content-loss regression; records the packUsageCellRows bound, the page-count saturation table, the it.failing tripwire trap (a helper baking in the buggy assumption), the NBSP-must-be-escaped-in-test-expectations rule, literal-vs-invariant test layering, and the non-positive-budget hang +metadata: + type: project +--- + +# PR #1959 — inline meta content loss: the defect, the fix, and the tripwire trap + +## Status: RESOLVED (2026-08-03, branch `fix/1959-pdf-ux-green`) + +Production is fixed; the guard tests are normal `it` again. Everything below is the reusable lesson. + +## The defect + +`overviewPdf.ts` `buildOverviewContent` appended `areaText`/`attachmentsNote` as ONE **unchunked** +grey run on a usage-chunk row's cell. With `table.dontBreakRows: true` pdfmake does not paginate an +over-tall row — it measures the text and then **silently drops** whatever doesn't fit. #1929 round 4 +had bounded exactly this by giving each field its own chunked continuation row; #1959's inline move +removed the bound without replacing it. + +**Why it happened:** the PR body asked for inline rendering and the implementer took it literally +without re-bounding the *combined* cell height. `attachmentsNote` has no maxLength anywhere (editor +or server) and `areaText` is aggregate-unbounded across N leaf areas, so ordinary user data reached it. + +**Measured evidence** (real unmocked renders, production 6-column shape). Rendered *line* count kept +growing linearly (~0.023 lines/char) while page count saturated — proof pdfmake measured everything +then discarded it: + +| chars | usageText (chunked) | attachmentsNote — before fix | after fix | +| --- | --- | --- | --- | +| 2000 | 6 rows / 2 pages | 3 rows / 3 pages | 6 rows / 2 pages | +| 4000 | 9 rows / 3 pages | 3 rows / **2** pages | 9 rows / 3 pages | +| 8000 | 15 rows / 5 pages | 3 rows / **2** pages | 15 rows / 5 pages | +| 16000 | 27 rows / 9 pages | 3 rows / **2** pages | 27 rows / 9 pages | + +Page count was even non-monotonic (3 → 2 as content grew). Reproduces the architect's original tell +verbatim: "rows requiring 3 and 9 pages both rendered as 2". + +## The fix, and what it changed for tests + +`packUsageCellRows(segments, maxChars)` packs the cell's **whole** content stream (prose, then the +grey meta segment) into per-row groups of ≤ `MAX_SAFE_USAGE_CHUNK_CHARS`. Lossless; byte-identical +output when the whole cell fits one row (the dominant case). + +Two consequences tests must encode, not work around: + +1. **The suffix now lands on the LAST row, not the first.** Pinning it to row 0 rendered grey meta + mid-prose with more usage below it, and shrank the prose's chunk boundary (adding rows). +2. **The suffix can span many rows** — one grey run per row, always last within its own cell. So + reconstructing it means concatenating the grey run of *every* row in the group. + +`MAX_SAFE_SMALL_CHUNK_CHARS` and `SMALL_SAFE_TOKEN_CHARS_7COL/_6COL` were 9pt ceilings for the +continuation rows that no longer exist; the meta renders at 8pt `tableCell`, so the usage budget is +the right one. All three (plus the orphaned `SMALL_WORST_CASE_CHAR_WIDTH_PT`) were deleted along with +their imports and value-only tests. `TABLE_SMALL_FONT_SIZE` stays — `PDF_STYLES.small` still renders +footnotes, cover-letter date/reference lines, and the running header/footer. + +## Technique: `it.failing` as a tripwire for a known-open production defect + +Jest 30 here supports `it.failing` / `test.failing`. It is the right primitive when QA must leave a +suite green but must not bless broken behaviour: the suite reports **passed** while production is +broken, and the moment production is fixed Jest errors with `Failing test passed even though it was +supposed to fail`, forcing a flip back to `it`. Verify the guard is live by temporarily relaxing its +threshold and confirming that error appears. + +### The trap that nearly disarmed it — check this every time + +**A tripwire is worthless if a shared helper bakes in the very assumption the bug lives in.** Here +`renderCellScopeRow` derived its expected row count from `usageText` alone — i.e. it asserted "the +meta never adds rows", which is precisely what the bug did. Any correct fix *must* add rows, so the +helper threw on its own row-count assertion before the test reached its page-count assertion: the +test failed for the wrong reason and `it.failing` stayed green through the fix. + +Rules that follow: +- Derive expected shape from the **same function production uses** (here `packUsageCellRows`), never + from a re-derivation of one input channel. +- Assert losslessness and budget bounds against the **INPUT and the declared constant**, never + against the packer's own output — otherwise a packing regression satisfies them by moving in step. +- After flipping a tripwire, confirm it passes **on its own assertion**, not merely that the suite is + green. + +## Strongest formulation found: channel equivalence + +The assertion that needed no calibrated threshold and is immune to future layout tuning: +**the same text costs the same number of pages whichever channel carries it** (`usageText` vs +`attachmentsNote`). Pre-fix this failed spectacularly (9 pages vs 2 at 16,000 chars). Pair it with a +per-channel losslessness check first, so "same page count" can't be satisfied by both channels +dropping equally. + +Related: tree-level assertions **cannot see this bug class at all** — the dropped text is present in +the pdfmake content tree while the reader receives a truncated PDF. Where the claim is about what the +reader receives, assert against rendered output (page count). + +## NBSP in inline PDF labels — and why a test expectation must escape it + +`ux-designer` found `depositReducedInlineLabel` was the only inline label in either locale with an +internal space, so at 8pt in the 75pt allocated-amount column it wrapped at that space and split the +brackets across lines (`€4,000.00 (less` / `deposit)` EN; `(Teilbetrag) (abzgl.` / `Abschlag)` DE) — +in a document sent to a bank. Both locales now use U+00A0; same glyph advance, so no width or +geometry constant moved. + +**In test expectations, write `\u00A0` as an escape, never paste the literal.** A literal NBSP and a +literal `' '` are visually identical, so a failure reads as a flake, and the "obvious fix" — retyping +a plain space — silently restores the wrap **with the unit suite green**, because the locale file and +the expectation would agree with each other again. The escape is the only thing that makes the diff +reviewable. (The repo convention of literal non-ASCII with zero `\uXXXX` escapes is about +`client/src/i18n/**` *JSON*; it does not extend to test expectations, where visibility wins.) + +I violated this while writing the guard against it: the invariant test's own +`c !== ''` filter went in with a literal NBSP instead of `'\u00A0'`. It passed, and was invisible. +**Always scan a file's codepoints after writing NBSP-related assertions** — +`node -e '[...fs.readFileSync(f,"utf8")].filter(c=>c.codePointAt(0)===0xA0)'`. + +### Literal vs invariant: keep both, at different levels + +Asked whether to replace the brittle literal with "contains no U+0020", the answer was both, because +they catch disjoint things: + +- The **literal** in `realRender.test.ts` pins WIRING — that the deposit-reduced label of the *report* + language reaches the allocated-amount cell. It catches a cross-key/cross-locale mix-up (rendering + `splitNote` where `depositReducedNote` belongs). A "contains no space" check passes happily while + the wrong label renders. +- The **invariant** in `client/src/i18n/i18n.parity.test.ts` pins TYPOGRAPHY, survives copy edits + (verified: rewording to `net ofdeposit` still passes), and covers every locale and every + future label automatically — the real regression surface. + +Scope it carefully: only keys that exist *solely* as bracketed inline labels +(`splitInlineLabel`, `depositReducedInlineLabel`) get the hard no-U+0020 rule. +`attachmentType.deposit` is **shared** — `buildReportContent.ts` also comma-joins it into flowing +attachments-note prose, where NBSP would be over-reach — so it gets the weaker "is a single word" +guard, which still fires if a translator makes it multi-word. The invariant rejects U+202F (narrow +NBSP) too, deliberately: it is non-breaking but has a *narrower advance*, and "same advance, no +geometry moved" is what made the fix safe to land unmeasured. + +## Non-positive chunk budgets hang rather than throw (now guarded) + +`splitIntoPageSafeChunks` with `maxChars <= 0` used to **spin forever**, not fail: the hard-split path +does `token.slice(0, maxChars)`, and `slice(0, 0)` is `''`, so `rest` never shrinks (probe: 100,000 +iterations, `rest` unchanged). `packUsageCellRows` has the same shape — I confirmed it independently +by replaying its loop bounded, and a mutation probe removing its guard **hung the jest runner**, which +is itself the proof. Both now throw. When probing a guard whose absence is non-termination, replay the +loop with a spin counter instead of running the suite, or the probe eats the timeout. + +Two subtle guard tests earned their place under mutation: an off-by-one (`<= 1`) is caught only by +asserting the smallest *legal* budget (1) still works, and moving the guard after the +`text.length <= maxChars` short-circuit is caught only by testing a non-positive budget with input +short enough to short-circuit. + +## Mutation-probing JSON: target the path, not the string + +`s.replace('"deposit": "Deposit"', ...)` hit a *different* `deposit` key in another scope of the same +file, so the probe reported a false negative on the guard. Mutate through +`JSON.parse` → set the exact dotted path → `JSON.stringify`, and print the resulting value to confirm +the mutation landed where intended. + +## Gotcha: the grey meta run is never at a fixed run index + +`buildUsageTextRuns()` tokenizes prose into **one run per whitespace-delimited token**, so a cell's +`.text` for `'Kitchen work'` is `[{text:'Kitchen'},{text:' '},{text:'work'}]` and the meta run is at +`text[3]`, not `text[1]`. Locate it by its colour (`#6b7280` = `DEPOSIT_NOTE_TEXT_COLOR`), assert it +is **last within its cell**, and reconstruct prose from the runs before it — see `splitUsageCell()` +in both `overviewPdf.test.ts` and `realRender.test.ts`. Note the production code strips the leading +`'\n'` when the suffix *starts* a cell (its own continuation row), so keep both a raw and a stripped +accessor for faithful cross-row concatenation. + +## Environment gotcha: `grep` silently returns nothing on these test files + +After `npx prettier --write`, `grep` (even `grep -c ""`) on `overviewPdf.test.ts` and +`ReportContentEditor.test.tsx` returns **no output and no error** — binary-content detection on some +byte sequence. This produced a false "constant is unused" conclusion mid-task. Use +`awk '/pattern/{print NR": "$0}' file` instead whenever a grep result on a report-PDF test file looks +suspiciously empty. `npx eslint` is the reliable authority on unused imports. + +Related: [[test-infra-reference]], [[story-1929-round2-real-render-technique]], +[[story-1923-report-table-cleanup]], [[story-1898-report-table-refinements]] diff --git a/.claude/agent-memory/translator/MEMORY.md b/.claude/agent-memory/translator/MEMORY.md index 5c74d3d50..667997067 100644 --- a/.claude/agent-memory/translator/MEMORY.md +++ b/.claude/agent-memory/translator/MEMORY.md @@ -97,6 +97,11 @@ New `sourceReports.expand.*` (chevron-expand sub-tables for budget lines + depos - [Progress/status label style](progress-label-style.md) — two co-existing patterns for "X-ing… (Ns)" labels; pick the closer sibling key as precedent (`autoItemize.analyzing` → "Analysiere… ({{seconds}}s)") - [EMPTY_SELECTION backfill](empty-selection-error-code.md) — pre-existing error code with no locale entry in either en or de; verify via server code before assuming a key is "new" +## PDF Inline-Label Constraints (#1917/#1959) + +- [NBSP for inline labels](nbsp-inline-labels.md) — multi-word inline PDF labels need U+00A0, not a regular space, or pdfmake wraps mid-bracket; includes the ad-hoc real-render/pdftoppm verification recipe +- [Abschlag glossary short-form](abschlag-glossary-shortform.md) — PO-approved `Abschlag` short form recorded IN glossary.json (75pt column has no room for any qualifier); `split`'s 3 German forms deliberately un-pinned + ## 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 @@ -106,8 +111,9 @@ New `sourceReports.expand.*` (chevron-expand sub-tables for budget lines + depos `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. + +- `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/translator/abschlag-glossary-shortform.md b/.claude/agent-memory/translator/abschlag-glossary-shortform.md new file mode 100644 index 000000000..651b6a8f1 --- /dev/null +++ b/.claude/agent-memory/translator/abschlag-glossary-shortform.md @@ -0,0 +1,33 @@ +--- +name: abschlag-glossary-shortform +description: "Abschlag" is a PO-approved short-form of the glossary term "Deposit" (Abschlagszahlung), permitted only under a measured space constraint — recorded in glossary.json itself +metadata: + type: project +--- + +Issue #1917/#1959: `product-owner` ruled that `"Abschlag"` (short form) may be used instead of the +glossary term `"Abschlagszahlung"` for the **deposit-reduced inline label** +(`depositReducedInlineLabel`: "abzgl. Abschlag") in the report PDF's Allocated-amount table column. + +**Why there's no third option** (the PO's decisive point): the column budget is 75pt. +`"Abschlagszahlung"` alone already consumes 72.85pt — so literally no qualifier of any length fits +alongside it, not `abzgl.`, not `ohne`, not `./.`. Keeping the full term therefore forces dropping +the qualifier entirely, which would silently collapse this label into the visually-similar but +semantically-different **constituted**-deposit label (`(Abschlagszahlung)`, #1923 AC2.1). Those are +different facts on a Verwendungsnachweis: "this row **is** a deposit" vs. "this amount is **reduced +because** deposits were claimed separately" (context restored by #1965's revived footnote, +`depositReducedFootnote` — do not delete that key). + +**Recorded directly in `client/src/i18n/glossary.json`** under the `Deposit` term's `de.shortForm` / +`de.shortFormNote` fields (not just in this memory file) — the point is that a future glossary +compliance sweep reading `glossary.json` alone must see the 75pt-column-budget reasoning and refuse +to "fix" `abzgl. Abschlag` back to `abzgl. Abschlagszahlung` in good faith. See +[[nbsp-inline-labels]] for the companion fix (NBSP inside the short-form label itself, to stop it +wrapping mid-bracket) that this same PR round also required. + +**`split`'s three German forms — deliberately NOT glossary-pinned**: `anteilig` (adjective, e.g. +"anteilig belassen") / `Anteil` (noun, "...zugeordneten Anteil") / `Teilbetrag` (noun, +`splitInlineLabel`). The PO ruled the glossary exists to prevent _semantic_ divergence across +translators/time, not surface-form variation — pinning one form here would force ungrammatical copy +across an adjective and two nouns that all mean the same thing in context. Recorded on #1917 as +"reject if proposed later" — do not re-propose a `Split`/`anteilig` glossary entry. diff --git a/.claude/agent-memory/translator/nbsp-inline-labels.md b/.claude/agent-memory/translator/nbsp-inline-labels.md new file mode 100644 index 000000000..e1805e1df --- /dev/null +++ b/.claude/agent-memory/translator/nbsp-inline-labels.md @@ -0,0 +1,45 @@ +--- +name: nbsp-inline-labels +description: Multi-word inline PDF labels must use U+00A0 (NBSP) instead of a regular space to prevent mid-bracket line wraps in pdfmake tables +metadata: + type: project +--- + +Issue #1959 (PR review round): `depositReducedInlineLabel` (`de/budget.json`: "abzgl. Abschlag") was +the only inline report-table label containing an internal space — every sibling label +(`Teilbetrag`, `Abschlagszahlung`, `(Rückerstattung)`) is a single word. pdfmake wraps at any space +by default, so this one broke across two lines with the opening/closing bracket separated — +unacceptable in a document sent to a bank. + +**Fix applied**: replaced the regular space between "abzgl." and "Abschlag" with a literal U+00A0 +(non-breaking space, UTF-8 bytes `c2 a0`) in the JSON string — not a ` ` escape. Confirmed via +`xxd` that the EN sibling key (`depositReducedInlineLabel`: "less deposit", owned by +frontend-developer) already uses the same literal-NBSP convention in this repo, not the backslash +escape form. NBSP has the identical glyph advance to a regular space, so this is a **zero-width-cost** +fix — no page-geometry/column-width constant needs to change, and any prior width measurement in pt +remains exactly valid. + +**Rule going forward**: any new multi-word inline label destined for a narrow pdfmake table column +(report PDF overview/cover-letter tables) must use NBSP between its words, not a regular space, +UNLESS the column is verified wide enough that the full label plus its preceding run (amount + +label share one run stream) never approaches the column's declared pt width. Do not rely on a +label-alone width calculation — pdfmake lays out the amount and the label as one continuous run, so +the combined width is what determines wrapping, not the label's width in isolation (see +[[abschlag-glossary-shortform]] for the width-budget details of this specific case). + +**Verification method**: width arithmetic alone missed this bug (see the same lesson at +[[audit-pitfalls]] for the analogous "measured the wrong quantity" pattern in a different context). +The only verification that caught it was rendering the _real_ pdfmake pipeline end-to-end and +looking at the PDF. Reusable recipe (no committed harness exists for this — built ad hoc, deleted +after use): + +1. Write a temporary `*.test.ts` file under `client/src/lib/reportPdf/` (Jest picks it up + automatically; delete it before handoff — it is not a real test, just a rendering harness). +2. Reuse the real, unmocked pipeline exactly as `realRender.test.ts` does: real i18next instance + loaded with the actual `en/budget.json`/`de/budget.json`, real `buildReportContent` + + `generateReportPdf` (from `merge.ts`), real `loadPdfLibs()` (real pdfmake@0.3.11 + embedded + fonts). Needs the `Blob.prototype.arrayBuffer` FileReader polyfill from that file's `beforeAll` + (jsdom's Blob polyfill lacks `.arrayBuffer()`). +3. `fs.writeFileSync('/tmp/x.pdf', Buffer.from(await result.blob.arrayBuffer()))`. +4. `pdftoppm -png -r 200 /tmp/x.pdf /tmp/x` then `Read` the resulting PNG to visually inspect. +5. Run with `NODE_OPTIONS=--experimental-vm-modules npx jest --maxWorkers=1`. diff --git a/.claude/agent-memory/ux-designer/MEMORY.md b/.claude/agent-memory/ux-designer/MEMORY.md index a95b754a5..754bfa7b8 100644 --- a/.claude/agent-memory/ux-designer/MEMORY.md +++ b/.claude/agent-memory/ux-designer/MEMORY.md @@ -36,3 +36,6 @@ - A component that renders its own internal `` (e.g. `TriStateCheckbox`), when wrapped in an *outer* `