Skip to content

fix(reports): improve report PDF UX — paragraph breaks, inline meta, inline notes, column toggles - #1959

Merged
steilerDev merged 6 commits into
betafrom
fix/report-pdf-ux-improvements
Aug 3, 2026
Merged

fix(reports): improve report PDF UX — paragraph breaks, inline meta, inline notes, column toggles#1959
steilerDev merged 6 commits into
betafrom
fix/report-pdf-ux-improvements

Conversation

@steilerDev

Copy link
Copy Markdown
Owner

Summary

  • AI cover letter paragraph breaks: Body text is split on double newlines into separate pdfmake content blocks; each paragraph gets proper inter-paragraph spacing (8pt gap, 20pt trailing margin)
  • Area and attachments inline in usage cell: areaText and attachmentsNote are now rendered as a grey inline suffix (\n EG · 1 Anhang) within the usage cell instead of a separate pdfmake stack sub-row; same applies to the ReportContentEditor UI (attachments column removed, content moved inline)
  • Footnotes → inline labels: (split) and (deposit-reduced) footnote markers replaced with inline grey (Teilbetrag) / (abzgl. Abschlag) labels appended to the allocated amount cell, matching the existing deposit note pattern. allocatedMarkers: string removed from ReportContentRow; replaced with isSplit: boolean and isDepositReduced: boolean
  • Column visibility toggles in ReportContentEditor: Checkboxes above the summary table let the user show/hide each column (local state, no persistence needed)

Test plan

  • Lint passes (npm run lint — no new errors in touched files)
  • Format clean (npm run format:check)
  • All buildReportContent.test.ts, overviewPdf.test.ts, realRender.test.ts, ReportContentEditor.test.tsx, applyAiContent.test.ts, applyOverrides.test.ts pass CI
  • Verify PDF export renders paragraph breaks in cover letter
  • Verify area/attachments appear grey inline under usage text in both PDF and UI preview
  • Verify split/deposit-reduced show inline (partial) / (less deposit) labels instead of / footnotes
  • Verify column toggle checkboxes work in the ReportContentEditor summary tab

🤖 Generated with Claude Code

@steilerDev
steilerDev force-pushed the fix/report-pdf-ux-improvements branch from 2175ab2 to 23c3537 Compare August 3, 2026 11:54
steilerDev and others added 2 commits August 3, 2026 17:05
…inline notes, column toggles

- AI cover letter body now splits on double newlines into separate pdfmake
  content blocks, preserving paragraph breaks from the LLM response
- Area and attachments count moved inline into the usage cell (grey, same
  font size) instead of a separate stack/sub-row
- Split (†) and deposit-reduced (‡) footnotes replaced with inline grey
  labels in the allocated amount cell, matching the existing deposit pattern;
  allocatedMarkers field removed from ReportContentRow in favour of boolean
  isSplit / isDepositReduced flags
- ReportContentEditor gains per-column visibility toggles (local state, no
  persistence); attachments column removed — content is now inline under usage

Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
…ontent

The inline-meta change appended areaText/attachmentsNote as one unchunked
run on the usage cell. With table.dontBreakRows, pdfmake does not
paginate an over-tall row — it silently discards the overflow. This
reintroduced the #1929 round-4 content-loss regression: page count
saturated at 2 and went non-monotonic (3 -> 2 as content grew) while
rendered line count kept rising linearly, so ~7 pages of measured
content were thrown away. Reachable from ordinary data: attachmentsNote
has no maxLength and areaText is unbounded across leaf areas.

packUsageCellRows() now bounds the whole rendered cell stream rather than
usageText alone, splitting via the existing splitIntoPageSafeChunks().
Page count is monotonic again and identical whichever channel carries the
text. Where the cell fits one page-safe row — the dominant case — output
is byte-identical.

The grey suffix now lands on the last chunk row rather than the first:
pinning it to row 0 rendered meta mid-prose with more usage below it, and
forced the usage chunk boundary to shrink.

Also updates the tests that encoded the unbounded model. Two of them
passed while the PDF lost content, because they inspected the pdfmake
content tree rather than rendered output; the guard is now framed as
rendered-output-only and asserts that the same text costs the same number
of pages whichever channel carries it. Adds coverage for packUsageCellRows
(none existed), the inline split/deposit labels, and the column toggles.
Removes the small-chunk constants, now dead.

E2E: reportWizard scenarios updated for the renamed usage-meta class, the
removed attachments field, and the footnote-to-inline-label change.

German strings verified against the glossary; abzgl. Abschlag kept over
the approved Abschlagszahlung because the compliant term measures 96pt
against a 75pt column.

Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com>
Co-Authored-By: Claude translator <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

[product-architect]

VERDICT: APPROVED

Architecture review of the fix round on head 9eaa9295. The content-loss fix is correct and, more importantly, correctly located — I verified it independently rather than taking the measurements on trust. No blocking findings. Five non-blocking findings below; two of them I recommend closing before #1958 promotes, and they are both cheap.

I did not re-verify test coverage per your note. I did independently re-derive the packer's invariants.


Independent verification of packUsageCellRows

Fuzzed the packer's contract directly (3,000 randomized inputs, budgets 1–80, mixed whitespace, meta present/absent):

  • Losslessness — concatenating every returned segment reconstructs the input byte-for-byte: 0 failures. This holds because splitIntoPageSafeChunks keeps whitespace as its own capture-group tokens, so tail.join('') in the used > 0 branch is safe. That dependency is load-bearing and undocumented at the call site — worth one comment.
  • Per-row budget — no emitted row's total character count exceeds the budget: 0 failures.
  • "At most one grey run per row, always last"0 violations across 3,000 trials. This is guaranteed structurally, not incidentally: every push inside a segment's loop is followed by flush() except the terminal break, so one segment contributes at most one piece per row.

That last point turned up the most useful thing in the review, see finding 1.


1. Is packUsageCellRows the right shape? — and what stops round 5

The shape is right, and for a reason worth naming explicitly, because it is the reason round 4 failed and this won't in the same way.

Round 4's bound was a layout invariant: "each field gets its own row." Enforcement lived in the row-emission code, so any later layout change silently detached it — which is exactly what #1959 round 1 did. #1959's bound is a data invariant: "the cell's content stream is packed against one budget," enforced where the stream is assembled. Push a segment into cellSegments and it is bounded automatically. That is the correct locus, and the doc comment's framing ("the bound now follows the rendered cell rather than a single field") is the right generalization.

On your real question — an assertion that fails on the next attempt to add a channel, rather than being caught by measurement. Three layers; one already exists, one is a genuine gap, one is optional.

Layer 1 — already present, and stronger than the PR claims. splitUsageCell throws on greyIndexes.length > 1. I tested the hypothetical: feeding the packer two meta segments (a plausible future "area" + "attachments" split, or any second grey channel) produced a row with two grey runs in 983 of 2,000 fuzzed inputs. So a second grey channel is caught loudly, by an existing assertion, with no test edit. Credit where due — that guard is doing real work.

Layer 2 — the gap, and the answer to your question. expectEveryRowWithinPageSafeBudget counts characters generically over all runs (typed.map(r => r.text).join('')), so a new non-grey channel would be counted correctly. But the inputs are hand-enumerated: renderCellScopeRow({ usageText, areaText, attachmentsNote }). A new ReportContentRow string field defaults to empty, so every existing assertion passes vacuously. The assertion is generic; the input generation is not. That asymmetry is precisely why each round needed fresh measurement.

The fix is small and mechanical — derive the adversarial input from the row's own keys:

const saturated = Object.fromEntries(
  Object.entries(baselineRow).map(([k, v]) =>
    [k, typeof v === 'string' ? proseOfLength(4000) : v]),
) as ReportContentRow;

Then run the existing per-row budget, losslessness, and monotonic-page assertions over it. A dev adding a fourth channel gets it saturated for free. If they wire it into the Usage cell outside the packer, the budget assertion fires — without anyone remembering the test exists. This is the single change that converts "caught by measurement each round" into "caught by the suite," and it is the one thing I'd most like to see land.

Layer 3 — optional, compile-time. buildUsageCell returns plain Content, and rows.push([...]) accepts any Content in the last slot, so hand-building a usage cell (or appending a run to a packed one) is legal TypeScript. A branded return type — type PackedUsageCell = Content & { readonly __packed: unique symbol }, with the row tuple's last element typed to require it — makes bypass a type error at zero runtime cost. Worth it given the cost history; not required.

One structural note: the "at most one grey run, always last" invariant currently lives only in test code. That is defensible for a pure builder, but it means the invariant is documented in a test helper rather than at the boundary that must maintain it. Exporting it as a predicate from overviewPdf.ts that the helper calls would put it next to the code that can break it.

2. Meta on the LAST chunk row — sound, and it is the only placement consistent with the bound

Agreed, and the justification in the PR undersells itself. Pinning meta to row 0 forces the packer to reserve its budget up front, shrinking the first usage chunk. Concretely: at a 650 budget with a 100-char meta, row 0 takes only 550 chars of prose — so a 650-char usageText, which #1929 AC12 requires to render in one row with zero degradation, would split into two rows purely because the invoice's item happened to resolve to an area. That is an AC regression triggered by unrelated data, and strictly worse than the placement change.

The trailing position is also semantically correct: EG · 1 Anhang is a suffix, and reading it as a trailing annotation beats grey text interrupting black prose that continues below it.

On visibility: the change only differs from pre-fix output above ~650 chars of usage, and AC12's own zero-degradation floor is 600 — the product has already declared 600+ the exceptional tail. So the visible delta is confined to rows the product treats as outliers.

One thing for UAT, not for code: on a continuation row the meta sits in a row whose leading/amount cells are blank, so a reader scanning the Allocated Amount column sees EG · 1 Anhang with no invoice identity beside it. Pre-fix it sat next to the invoice. That is a legibility trade in a bank-facing document, it is rare (650+ chars), and it is a human call — worth one glance during UAT rather than a code change.

3. Losing content.footnotes — separate the mechanism from the content

Mechanism (non-blocking, follow-up). Dropping the rendering is fine; keeping the channel alive while guaranteeing it empty is not. buildReportContent now emits [] unconditionally, yet four consumers still branch on or copy it (overviewPdf.ts:833, ReportContentEditor.tsx:445, and the spreads in applyOverrides.ts:27 / applyAiContent.ts:26); ReportContentFootnote.marker is unreachable; and the E2E page object had to add a prose warning ("never assert a positive count on these"). A type permitting a state the producer cannot emit, with the constraint enforced by comment, is the same shape as finding 4 — see the theme note below. Remove ReportContent.footnotes, ReportContentFootnote, the editor block, and the overviewPdf branch; that also deletes the need for the E2E warning.

I verified the skip-footnote block is built independently from skipFootnotesByInvoiceId and is unaffected — #1923 AC2.5 holds. Nothing else needs the channel.

Content — this is the part I would escalate. †: Amount shown reflects only the portion allocated to this source.(partial) is roughly meaning-preserving. ‡: This position reflects deposits claimed separately.(less deposit) / (abzgl. Abschlag) is not. The footnote told the bank the deposit was claimed in a separate submission — material to not double-funding. (abzgl. Abschlag) says only that the amount was reduced. In a bank-facing document that is a semantic loss, not a terser label.

I am not the authority on that (product-owner is), but it should be a recorded ruling rather than a side effect of a UX polish pass — see finding 6.

4. applyOverrides still honouring row.<id>.attachmentsNote — dead, remove it

Dead code. Verified there is no back-compat reason to keep it:

  • Overrides are ephemeral wizard state — no server persistence. No report_override / content_overrides table, nothing across migrations 00010044. So no saved override can carry the key.
  • applyAiContent writes only usageText and cover-letter fields. So no generated override can carry it either.

With the editor's input gone, the key is unreachable from every producer. Remove the rowKeys.attachmentsNote branch, its doc line in applyOverrides.ts:4, and the entry in ADR-034's override-key list (line 148).

Explicitly do not keep it "as a capability." A write path with no reader and no producer is how the round-3/round-4 confusion started: the code said one thing and the comment said another.

5. Column visibility toggles — the affordance does not match the label

Flagging this because it is the one user-visible issue and it is in a bank-facing workflow. hiddenColumns is local useState in ReportContentEditor; it is never lifted, persisted, or plumbed to buildOverviewContent. Hiding a column changes the preview only — the exported PDF still contains every column.

What makes this a trap rather than a nit: the control is labelled plainly Show/hide columns, it sits next to the table heading, it lists exactly the PDF's column set, and every sibling control in that editor (usage text, cover-letter fields) does flow through to the PDF via overrides. The mental model it teaches is wrong. Mitigating factor, and the reason this is not blocking: the wizard renders a live PDF preview, so a user would see the column still present.

The architectural constraint that matters here — and the reason this is my call and not purely a UI question: USAGE_WIDTH_7COL/_6COL derive from usableColumnWidth(n), and MAX_SAFE_USAGE_CHUNK_CHARS (650) was measured against the 7-column geometry. Making toggles affect the PDF widens the Usage column and invalidates the measured ceiling. So "just plumb it through" is not a small change — it requires re-measuring the ceiling, i.e. re-entering exactly the loop that cost three rounds.

Given that, my recommendation is the cheap correct one: label the control preview-only (or drop it from this PR), and file real column selection as its own story that must re-derive the usage ceiling. I am recording this constraint in ADR-034 either way so the next author does not discover it the hard way.

Two lesser notes, both low: unit coverage of the toggles is genuinely thorough (parameterised over every column, both directions, independence verified) — but there is no E2E coverage, deliberately skipped and documented in the page object to avoid reshuffling shard membership. Given CLAUDE.md's E2E page-coverage requirement that is a real if minor gap. And ux-designer is a required reviewer here: a one-off inline checkbox group, rather than a shared component with a visual spec, touches the Component Reuse Policy. That verdict is theirs, not mine — I note it only so it is not lost.

6. Acceptance-criteria record — please close this before #1958 promotes

Not a code finding, but the thing most likely to cause trouble later.

#1923 shipped 2026-08-02 in 2.13.0-beta.38. #1959 reverses its AC1.1, AC1.2, AC2.3, AC2.4 (and AC3.4's "footnotes are all still rendered") one day later. AC1.2 mandated verbatim: "it contains exactly one entry for splits: †: Amount shown reflects only the portion allocated to this source." #1923 remains CLOSED with those criteria marked delivered.

Meanwhile #1959 has zero comments: no product-owner authorship (its body is a PR-style summary, not a PO story with acceptance criteria — contrast #1923's **[product-owner]** header and numbered ACs) and no ux-designer visual spec (#1923 had one). So the record currently asserts two contradictory behaviours are both correct on beta, and #1958 will summarize #1923's ACs as delivered when four of them are now false.

The fix is two comments, and it also settles the (abzgl. Abschlag) semantic question from finding 3:

  1. PO comment on Report table cleanup: shared footnotes, inline deposit labels, claim metadata, total-only summary, area in Usage #1923 marking AC1.1 / AC1.2 / AC2.3 / AC2.4 superseded by fix(reports): improve report PDF UX — paragraph breaks, inline meta, inline notes, column toggles #1959 (AC2.5 is intact — verified).
  2. PO ratification on fix(reports): improve report PDF UX — paragraph breaks, inline meta, inline notes, column toggles #1959 of the inline-label design, explicitly covering whether (less deposit) carries enough meaning for the bank.

7. packUsageCellRows hangs on maxChars <= 0 (low, but precise)

Confirmed empirically: packUsageCellRows([{ text: 'a' }], 0) loops forever. The remaining <= 0 → flush(); continue; branch is a no-op when current is empty, so the loop never advances.

This is new to the packer, not inheritedsplitIntoPageSafeChunks('a', 0) fails loudly instead (RangeError: Invalid array length), and the packer's own guard short-circuits before it is ever called.

Unreachable today (the argument is always the 650 constant), so: low. But it is worth a RangeError guard for one specific reason — it connects to finding 5. If column selection ever is plumbed to the PDF, the budget becomes computed rather than constant, and a computed budget is exactly the thing that can arrive as 0 or negative. A guard now costs one line and turns a future hang into a stack trace.

8. ADR-034 amendment — I own this and will do it

You are right that it needs one, and it is the highest-leverage durable artifact here: the ADR is the only place a round-5 author would look, and it currently says nothing about the failure mode that has now cost three rounds. Folding in the debt from #1929:

  1. New lesson: dontBreakRows: true silently drops an over-tall row's overflow. pdfmake measures the row, then PageElementWriter discards what does not fit — no throw, no visible truncation, page count saturates and can go non-monotonic (3 → 2 as content grows). Generalizable rule, stated to survive the next layout change: 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. Plus the two detection recipes that actually worked: page count must be monotonic in content size, and channel-independent (the same text costs the same paper whichever field carries it).
  2. Fix minimum-bar rule EPIC-01: Authentication & User Management #1. table._minWidth <= 515.28 is wrong and fails on correct code — pdfmake's _minWidth is the widest single unbreakable word, not the laid-out width, so a table that wraps fine can exceed it. Replace with max(horizontalRatio) <= 1. B2's narrative stays valid; it was the generalized rule that got mis-transcribed.
  3. Module table (drifted twice): add pageGeometry.ts (reportPdf geometry hygiene: bounds that name their own scope (HEADER_ROW_HEIGHT_MAX, char-advance scope, channel enumeration, PDF_STYLES relocation) #1939 — owns page/table geometry, font sizes, PDF_STYLES, usableColumnWidth) and index.ts; drop "PDF-local formatters" from shared.ts's role (deleted in review round 2 — it now holds colours, header/footer builders, TABLE_LAYOUT) and move "table layout constants" to pageGeometry.ts.
  4. Override-key list (line 148): drop attachmentsNote (finding 4).
  5. Record the column-count constraint from finding 5: the report's column set is fixed at 6 or 7, and MAX_SAFE_USAGE_CHUNK_CHARS was measured against the 7-column geometry, so column visibility is not free to plumb into the PDF.

Process note on myself, not on this PR: per my own standing rule the wiki update belongs in the story, not in the review that catches it. This PR should have carried the amendment. I am taking the action as a follow-up rather than pushing wiki changes onto this branch, since the ADR text is not a gate on the fix.


Summary

# Finding Severity Disposition
1 Budget assertion is generic over runs but inputs are hand-enumerated — a new channel passes vacuously Medium Key-driven saturation; the answer to "what stops round 5"
2 Meta on last row Sound. One legibility item for UAT
3 footnotes is a permanently-empty channel with 4 consumers Low Follow-up issue
3b (abzgl. Abschlag) loses "claimed separately" Medium Needs a PO ruling (with #6)
4 applyOverrides attachmentsNote is unreachable dead code Low Follow-up issue
5 Column toggles are preview-only but read as report-composition Medium Label preview-only or drop before promotion; design-system verdict is ux-designer's
6 #1923 ACs reversed with no PO ruling; #1958 will misreport them Medium Two issue comments before #1958 promotes
7 packUsageCellRows hangs on maxChars <= 0 Low Guard; matters only if #5 is ever plumbed
8 ADR-034 amendment + #1929 debt Medium Mine. Follow-up

Recommend closing before #1958 promotes: 5 and 6. Both are cheap — a label and two issue comments — and both concern what a bank actually receives or what the record claims was delivered. Everything else is a follow-up issue, and 1 is the one I would prioritise, because it is what stops this from becoming round 5.

One theme worth naming, since findings 3, 4 and 5 are the same shape: a capability retained in code, disabled at the producer, and documented by comment instead of by type. Empty-but-handled footnotes, an override key with no producer, a toggle with no effect on the output. Each is individually harmless; collectively they are why this module keeps needing archaeology. Deleting rather than commenting is the cheaper long-run move.

…ound rule

- Usage-cell height bound: bound what a cell RENDERS, never a source field
- Generic-assertion / hand-enumerated-input asymmetry as the recurrence cause
- Column-count constraint pinning MAX_SAFE_USAGE_CHUNK_CHARS to the 7-col shape
- Owed ADR-034 corrections (minimum-bar rule #1, module table, override keys)
- Two new recurring patterns: dead capability retained, AC reversal by polish issue

Co-Authored-By: Claude product-architect <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

[ux-designer] Design review of #1959 (head 9eaa9295).

Verdict: CHANGES_REQUIRED

One blocking finding, verified by real render. Everything else is non-blocking and listed below as follow-ups.

Method. I did not judge this from the diff. I rendered the real, unmocked pipeline (buildReportContentgenerateReportPdf, real pdfmake + real embedded fonts + real en/de budget bundles) to PDF via a throwaway test, rasterised at 130/200 dpi with pdftoppm, and inspected the images. Fixtures: short usage cell; usage >650 chars (forces chunking); area + attachments note together; multi-paragraph cover letter; every inline-label combination — in both locales. Scratch test deleted; git status clean.


BLOCKING — the (less deposit) / (abzgl. Abschlag) label splits mid-phrase

depositReducedInlineLabel is the only inline label in either locale that contains an internal space. Every other one — partial / Teilbetrag, Deposit / Abschlagszahlung, (refund) / (Rückerstattung) — is a single word and therefore wraps as an intact unit onto its own line. This one wraps at its internal space, orphaning the opening bracket on one line and the closing bracket on the next.

Rendered output, German, split invoice reduced by an untagged deposit (isSplit + budget lines + untagged deposit → both labels; entirely ordinary data):

        7.200,00 €
(Teilbetrag) (abzgl.
        Abschlag)

Rendered output, English, deposit-reduced without the (partial) label:

€4,000.00 (less
       deposit)

Both locales are affected. German breaks in the isolated and the combined case; English breaks in the isolated case. This is a regression introduced by this PR: the / markers it replaces were single glyphs that could not wrap.

The stated width rationale measured the wrong thing. The note carried into review says " (abzgl. Abschlagszahlung)" is 96.07pt against a 75pt column versus 63.95pt for " (abzgl. Abschlag)", so the shorter form was kept. But the label never renders in isolation — it always follows the formatted amount in the same run stream, and 4.000,00 € alone is already ~48pt. The combined width blows past the column regardless, so the label always wraps; the only question is whether it wraps whole or mid-phrase. Width was never the operative constraint. The presence of a breakable space is.

Fix, verified by render. Replace the space inside depositReducedInlineLabel with U+00A0 (non-breaking space) in both client/src/i18n/en/budget.json and client/src/i18n/de/budget.json. This preserves the 63.95pt measurement exactly — NBSP has the same advance width as a space — changes no geometry constant, and touches nothing but two string literals. I rendered it to confirm rather than assert it:

        7.200,00 €              4.000,00 €
      (Teilbetrag)        (abzgl. Abschlag)
(abzgl. Abschlag)

Each label intact on its own line, right-aligned, reading exactly like (Teilbetrag) already does. Apply the same substitution to the English label, replacing the single space in less deposit with U+00A0. To a reader and to the width calculation the string is unchanged; it simply stops offering a legal line-break opportunity.

If you would rather avoid NBSP in translation files, the alternative is a single-word label in each locale (e.g. Abschlag / net of deposit is still two words — it would need to be one). NBSP is the smaller and safer change, and it keeps the glossary-deviation reasoning already logged for abzgl. Abschlag untouched.


Confirmed good — the three judgement calls you asked about

1. Grey inline suffix vs. its own sub-row — the grey is strong enough. DEPOSIT_NOTE_TEXT_COLOR #6b7280 on white measures 4.83:1, clearing WCAG AA for normal text, and 3.04:1 against the adjacent body colour #1f2937 — above the 3:1 threshold for a perceptible non-text distinction. Because the separation is pure luminance, it survives greyscale printing unchanged. In the render Triple glazing / Ground Floor · 1 attachment: Invoice reads unambiguously as prose-then-metadata. Note the suffix is 8pt, the same size as the prose (DEPOSIT_NOTE_FONT_SIZE == TABLE_BODY_FONT_SIZE) — colour is doing all the work, and it is enough. Good call keeping it at body size rather than shrinking it.

2. Meta on the LAST chunk row is the right choice. Endorsed. On the continuation row the grey suffix lands directly after the final words of the prose, which is where a trailing annotation belongs. Row 0 would put grey metadata mid-sentence with black prose resuming below it in a different row — a reader would parse that as the description having ended and then restarted. The residual weakness you sensed is real but is not about placement: the continuation row has empty vendor/date/amount cells, so on either choice the meta sits in a row that is visually detached from its invoice. That is inherent to the #1929 chunking design, not to this PR, and it only occurs past 650 characters. Keep last-row.

3. Inline (partial) reads as an annotation, not part of the value. Confirmed in the render: grey, bracketed, and — because it wraps below the amount — visually subordinate to the black right-aligned figure. No risk of it being read as part of the number. (partial) is also self-evident in context because Invoice Amount €24,000.00 sits immediately beside Allocated Amount €9,600.00 (partial); the columns already tell the story the removed footnote sentence told. See follow-up 5 for the one case where that is not true.

Cover letter paragraph breaks: correct in both locales. Four paragraphs, 8pt inter-paragraph gaps clearly distinguishable from the ~18pt line height, 32pt before the signature block, salutation reading correctly as its own paragraph. Renders as a proper business letter.

Token adherence and dark mode in ReportContentEditor.module.css: clean. Every new value is a token (--spacing-*, --font-size-xs, --color-text-muted). No hardcoded colours, sizes, or radii — stylelint will pass. All colours route through semantic tokens that flip under [data-theme="dark"], so dark mode is correct by construction. role="group" + aria-label on the toggle container is the right ARIA pattern and matches the app's existing radio-group precedent. The new <td /> filler for status-less rows is a genuine column-alignment fix — good catch.


Non-blocking follow-ups

Ordered by how much I think they matter. None of these should hold the promotion — items 1–4 are better as separate issues than as changes to this PR.

1. The column toggles do not affect the generated PDF. (Medium — follow-up issue, but please decide consciously.) hiddenColumns is useState local to ReportContentEditor and never leaves it — no prop, no callback, and no consumer anywhere in client/src. The PDF always contains every column. This is the one finding I'd have escalated if the issue text hadn't said "local state, no persistence needed", so I accept it was intentional. My concern is the user's read, not the author's intent: this control sits directly above the heading "Report Table", inside a wizard whose sole output is a PDF, and every other control in that editor (EditableField) does change the PDF. "Show/hide columns" there will be read as "choose the report's columns". Either wire it through to buildOverviewContent, or make the preview-only scope explicit in the label/helper text. Filing it is fine; leaving it silent is the part I'd avoid.

2. The toggle group is a one-off, and a shared component for this already exists. (Medium — Component Reuse Policy rule 3.) client/src/components/DataTable/DataTableColumnSettings.tsx is the app's column show/hide UI (gear trigger, checkbox list, reset-to-defaults, Escape/outside-click handling). This PR builds a bespoke inline checkbox row instead. I accept it isn't a drop-in — that component is bound to ColumnDef<T> and DataTable.module.css, and it's desktop-only, whereas these toggles legitimately need to drive the mobile card list too. But policy rule 3 says a new pattern must still be extracted as a reusable shared component rather than living inline in a page component. Recommend extracting it (e.g. ColumnToggleGroup) in a follow-up, at which point the reset-to-defaults affordance DataTableColumnSettings has and this lacks comes along for free.

3. Toggle touch targets are well under 44×44px on mobile. (Medium — cheap enough to fix here if you like.) .columnToggle sets font-size: var(--font-size-xs) with a native checkbox and no min-height, giving roughly a 16–20px tall row. The toggles are not hidden by the @media (max-width: 767px) block, and they do control the mobile card rows, so they are genuinely interactive at mobile width. The house technique for this is .expandButton's padding-plus-negative-margin trick (small visible box, real 44px hit area); a min-height: 44px on .columnToggle at mobile width would also do it.

4. attachmentsNote is no longer editable anywhere. (Medium — follow-up issue.) Removing the column removed its EditableField too, so the row.<id>.attachmentsNote override key is now unreachable from the UI — types.ts documents this honestly, which I appreciate. Two consequences worth a ticket: users can no longer correct that text, and any override saved before this change still renders with no "edited" marker and no reset control. Restoring editability is a design change, not a tweak, so it doesn't belong in this PR.

5. (less deposit) lost its only explanation, and two i18n keys are now dead. (Low.) Unlike (partial), this label is not self-evident from the columns: a reader sees €18,400.00 invoiced and €7,200.00 (less deposit) allocated with no way to infer what was deducted or where it was claimed. The sentence that said so — depositReducedFootnote, "This position reflects deposits claimed separately." — is gone from the document while remaining in both locale files, along with splitFootnote. Suggest a follow-up that either restores a single-line legend under the table for the labels actually present, or deletes the two now-dead keys. Dropping splitFootnote is fine; dropping this one costs real information in a document whose purpose is to justify a disbursement.

6. .inlineNote is --font-size-xs but the PDF renders the same annotation at body size. (Low.) In the PDF, (partial) is DEPOSIT_NOTE_FONT_SIZE (8pt) == body size. In the editor it's xs, i.e. smaller than its cell. Meanwhile .usageMetaText dropped the font-size: var(--font-size-xs) that .usageAreaText used to carry, so it now does match the PDF. The result is two grey annotations at two different sizes within the same editor, only one of which matches the artifact it previews. Aligning .inlineNote to inherit would make the preview truthful and the editor internally consistent.


Pre-existing, out of scope — flagged only because this is on the path to main

Confirmed against the diff that this PR touches none of these. Not findings against #1959; worth knowing since the artifact goes to a bank.

  • The running page header prints a dangling label with no value. merge.ts:131 passes t('sourceReports.table.generatedAt') — literally "Generated" / "Erstellt am" — into buildPageHeader's generatedAtLabel slot with no date appended. Every page after the first shows Generated / Erstellt am ("Created on…") trailing into nothing. merge.ts is untouched by this PR. This one looks worth its own issue.
  • Vendor names break mid-word with no hyphen, on essentially every row: Miller Electri/cal Ltd, Nordwind W/indows, Keller Concr/ete. VENDOR_SAFE_TOKEN_CHARS resolves to 6 characters, so almost any real vendor name gets wordBreak: 'break-all' treatment. Known and accepted from Report PDF layout breaks: usage column overflows the page, rows split across page breaks, running header clipped #1929 round 4; re-confirmed here.
  • A trailing blank page appeared in one of my four fixtures (English claim, 9 rows: page 3 carried only the running header and footer, with the table comfortably clear of the page-2 boundary). I did not root-cause it and am not attributing it to this PR — but taller rows make boundary conditions more likely to be hit, so it's worth a look.
  • Dark-mode --color-text-muted (#64748b) on --color-bg-primary (#1a1a2e) is 3.59:1, below AA for normal text. App-wide and pre-existing; the new .columnToggle/.usageMetaText/.inlineNote rules inherit it rather than introduce it.
  • No color-scheme is declared anywhere in client/src/styles/, so native checkboxes render with light browser chrome in dark mode. Applies to every native checkbox in the app, not just these.

Re-request me once the label is unbreakable and I'll re-render both locales to confirm. Happy to open the follow-up issues if that's useful.

steilerDev and others added 3 commits August 3, 2026 18:39
- Footnote-to-inline-label reversal: labels kept, explanatory sentences
  restored as a report-level legend (#1965); supersession-comment policy
  for ACs on closed/released issues instead of rewriting them
- Glossary: Abschlag approved as a measured-space short form; split's
  three German surface forms deliberately get no entry

Co-Authored-By: Claude product-owner <noreply@anthropic.com>
- Multi-word inline labels split mid-phrase in narrow fixed-width PDF
  cells; an isolated width measurement cannot catch it. Verified U+00A0 fix.
- Check that a new report/export editor control actually reaches the
  export pipeline; DataTableColumnSettings already exists for column toggles.
- Compare preview-component styles against the exporter's constants.

Co-Authored-By: Claude ux-designer <noreply@anthropic.com>
depositReducedInlineLabel was the only inline label in either locale with
an internal space, so at 8pt in the narrow allocated-amount column it
wrapped there — putting the opening and closing bracket on different
lines. "EUR 4,000.00 (less" / "deposit)" in en, "(Teilbetrag) (abzgl." /
"Abschlag)" in de, in a document sent to a bank. Both now use U+00A0,
which has the same glyph advance, so no width or geometry constant moves.

The earlier width analysis measured the label alone against the 75pt
column. It always follows the formatted amount in the same run stream, so
the combined run exceeds the column regardless and the label always
wrapped: breakability was the operative variable, not width. Verified by
rendering a plain-space control from the same harness and reproducing the
break, then confirming it gone.

Adds a locale-level invariant so this cannot regress silently: the two
keys that exist solely as bracketed labels must contain no breaking
whitespace, checked against every locale. attachmentType.deposit is held
to a weaker single-word rule instead, because it is also comma-joined
into flowing prose where a non-breaking space would be over-reach.

Also guards packUsageCellRows and splitIntoPageSafeChunks against a
non-positive budget. Both hung rather than failing loudly — the packer in
its own loop, before it ever delegates — so guarding only the primitive
would not have fixed it.

Labels the ReportContentEditor column toggles preview-only: their state
never leaves the component, but every sibling control does reach the PDF,
so the label read as "choose the report's columns". Wiring is #1966.

Glossary records Abschlag as a measured-space short form of
Abschlagszahlung, with the 75pt budget as the reason, so a future
compliance sweep cannot silently break the PDF.

Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude translator <noreply@anthropic.com>
@steilerDev
steilerDev merged commit 3cc8967 into beta Aug 3, 2026
33 checks passed
@steilerDev
steilerDev deleted the fix/report-pdf-ux-improvements branch August 3, 2026 17:23
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

steilerDev added a commit that referenced this pull request Aug 4, 2026
…tWizardPage to useReducer

- **#1967** — Remove dead `attachmentsNote` override from `applyOverrides.ts` and `overrideKeys.ts`; field has been static read-only text since PR #1959, no UI path can produce its override key; adds regression test pinning the removal
- **#1947** — Refactor `ReportWizardPage` from 38 `useState`/`useRef` hooks to a `useReducer` state machine in `wizardReducer.ts`; staleness now enforced via opaque request-id tokens (M1/M2 fixes); named tier types with explicit factory return-type annotations enforce AC4 at compile time; 57 unit tests at 100% coverage; behaviour-preserving (existing tests unchanged per AC3)

Fixes #1967
Fixes #1947

Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude product-architect <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
steilerDev added a commit that referenced this pull request Aug 6, 2026
…1950)

- Recompute both derived Uk ceilings from the geometry constants rather than re-typing them, so `MAX_SAFE_USAGE_CHUNK_CHARS`'s documented 34-character exceedance fails loudly instead of going stale. The ceiling depends on `USAGE_WIDTH_7COL`, `TABLE_BODY_FONT_SIZE`, `TABLE_SMALL_FONT_SIZE` and `DEFAULT_LINE_HEIGHT`; before this, any of them could move while nothing failed and the only record of the exceedance was a comment a refactor could delete.
- Pin only the measured line budgets as literals, labelled as real-render measurements that cannot be derived. Each of the four constants was individually mutated to confirm the guard fires, and the failure message names the constant that moved and points at the rationale comment rather than reporting a bare number.
- Account for #1940's continuation-row marker, which adds two rendered characters no budget counts because it is applied post-packing. The base-row overage of 34 chars / 3 lines / 33.6pt becomes 36 / 4 / 44.8 there, and the extra line is bounded by asserting the marker is shorter than one line's capacity.
- Pin #1941's editor-renderer coupling — the usage-text input cap must stay below the renderer's 7-column budget — asserted from the rendered `maxlength` attribute, so no production export was needed.

Test-only: no production value changes and the rationale comments are untouched. The issue's AC 1.5 was struck on amendment, since the constant it names was removed by #1959 the day after the issue was filed; satisfying it literally would have meant restoring the exact thing this guard exists to keep from silently returning.

Fixes #1950

Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

1 participant