feat(reports): editable HTML report preview with on-demand PDF export - #1909
Conversation
Introduces a report content model (buildReportContent/applyOverrides) that produces a plain-string baseline merged with user overrides. Step 5 of the report wizard becomes an editable cover-letter and table surface built on a new EditableField shared component. PDF generation moves from live debounced regeneration to on-demand (preview modal, download, Paperless send), always built from the current edited content. Discards edits with a confirmation prompt when steps 1-4 change while overrides are present. Fixes #1900 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com> Co-Authored-By: Claude frontend-developer (Haiku 4.5) <noreply@anthropic.com> Co-Authored-By: Claude translator (Sonnet 4.5) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com> Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) <noreply@anthropic.com>
|
Verdict: REQUEST-CHANGES (posted as a comment — [product-architect] Architecture / code-quality / test-coverage review of PR #1909 (story #1900). Verdict: REQUEST CHANGES — CI What I verified as soundThe content/layout split is the right architecture and it is durable. Test architecture is excellent. Debounced-regen removal is clean. No leaked timers, no stale closures. Blob lifecycle is handled with a ref mirror + revoke on modal close + revoke on unmount, and jsdom's missing Blocking findingsB1 (Critical) —
|
|
[ux-designer] Design review against my visual spec on issue #1900 (comment link). Verdict: CHANGES_REQUIRED (Critical/High — two confirmed rendering bugs make core spec'd features non-functional in the real app, not just style nits) I read the full diff, Critical:
|
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner] Review of PR #1909 against story #1900 acceptance criteria.
Verdict: COMMENT — MUST FIX before merge
No functional acceptance criterion is unmet. The feature genuinely works end to end: Step 5 is an editable HTML surface, edits are overrides on a live baseline, every Step 1–4 change regenerates and clears with a correctly-gated confirmation, and all three export paths generate on demand from the edited content. The gaps below are display/formatting, token-adherence, and translation-correctness — per the verdict matrix those are --comment + MUST FIX, not --request-changes.
Note that Static Analysis is currently red (Stylelint, 13 errors), so this cannot merge regardless of my verdict. Items 1–3 below are exactly those errors.
AC Coverage
| AC | Criterion | Status | Evidence |
|---|---|---|---|
| 1.1 | Step 5 shows full report as editable HTML, not a PDF frame | ✅ | ReportContentEditor.tsx renders cover letter + table + summary + footnotes; E2E Sc.1 asserts iframe count is 0 |
| 1.2 | One row per included invoice with all report columns | ✅ | Table + mobile cards render vendor, invoice no., date, status (overview only), amounts, usage, attachments note |
| 1.3 | Step-3 exclusions absent from editable content | ✅ | baselineContent built from applyLineExclusions(report, excludedLineIds) + includedInvoiceIds filter |
| 1.4 | Cover letter on → 5 separately editable fields | ✅ | sender, recipient, reference, subject, body as EditableFields; reference/recipient omitted entirely when absent |
| 1.5 | Cover letter off → no fields, none exported | ✅ | coverLetter: null → editor renders nothing, merge.ts guards on if (reportContent.coverLetter) |
| 1.6 | Keyboard reachable, accessible label, visible focus, all viewports | ✅ | Real <input>/<textarea> in natural tab order; <label htmlFor> for letter fields, composed aria-label for table cells; focus ring survives via shared.module.css .input:focus-visible; E2E Sc.13 asserts box-shadow ring, Sc.11 mobile |
| 1.7 | Design tokens only, no hardcoded values, dark mode | ❌ | Stylelint red — font-weight: 500 hardcoded (item 2). See also item 1 |
| 2.1 | All wording fields freely editable | ✅ | usage text, attachments note, all 5 letter blocks |
| 2.2 | Amount cells read-only, visually + programmatically | ✅ | Rendered as plain <td> text — no form control exists at all, so nothing to focus or announce. ReportContentEditor.test.tsx:340 asserts queryByDisplayValue finds no input for the amounts |
| 2.3 | Totals/subtotals read-only, derived from Step-3 state | ✅ | summaryRows plain <td>; recomputed in buildReportContent from included set |
| 2.4 | No user input path can alter an exported number | ✅ | applyOverrides uses a hardcoded 7-key allowlist (coverLetter.{sender,recipient,reference,subject,body}, row.<id>.{usageText,attachmentsNote}); unknown keys silently ignored; summaryRows/footnotes never written. overviewPdf/coverLetterPdf now consume ReportContent only and no longer import SourceReportResponse |
| 3.1 | Baseline generated in the Step-4 report language | ✅ | reportT = i18n.getFixedT(reportLanguage,'budget') + reportFormatters threaded into buildReportContent |
| 3.2 | Step 1–4 change regenerates baseline and clears edits | ✅ | baselineContent useMemo keyed on report/useCase/exclusions/reportT/formatters/includeCoverLetter/household; confirm handler calls setOverrides({}) |
| 3.3 | Confirmation warning when edits exist, cancellable | ✅ | guardedUpdate wraps all Step 1–4 mutations: use case, source, invoice toggle, toggle-all, line toggle, report language, attach documents, include cover letter. "Keep Editing" clears pendingChangeRef without applying |
| 3.4 | No warning when there are no edits | ✅ | isDirty = Object.keys(overrides).length > 0; else applyChange() immediately. E2E Sc.5 |
| 3.5 | After confirm, content matches fresh baseline, no residue | ✅ | E2E Sc.4 |
| 3.6 | Untouched fields keep tracking the baseline (not frozen) | ✅ | effectiveContent = applyOverrides(baselineContent, overrides) recomputed each render — untouched keys are simply absent from the map |
| 4.1 | No continuous background regeneration, no live pane | ✅ | Debounced regeneration effect removed; hasBlob gating dropped from Step5Actions. E2E Sc.1 |
| 4.2 | "Preview PDF" generates from current edited content | ✅ | New leftmost btnSecondary → handlePreviewPdf → generatePdfFromContent() → modal |
| 4.3 | Download reflects edits | ✅ | Same single generatePdfFromContent() path; proven at unit level (see 4.6 note) |
| 4.4 | Paperless upload reflects edits | ✅ | Same path; E2E Sc.9 |
| 4.5 | Mark-claimed PDF reflects edits; #1891 warning unchanged | ✅ (vacuous — accepted, see below) | No PDF is generated by claiming; excluded-lines warning block preserved verbatim with role="alert". E2E Sc.10 |
| 4.6 | Preview asserts rendered content, not just a blob: src |
See below | |
| 4.7 | Generation failure surfaced, edited content preserved | ✅ | Preview → FormError in modal; download → showToast('error', downloadFailed); Paperless → toast. overrides untouched on every failure path |
| 5.1 | Every new string from t(), resolves in en + de |
❌ | 21 new keys added with exact en/de parity, but resetFieldAriaLabel is interpolated with raw English literals (item 4) |
| 5.2 | Chrome follows UI locale, content follows report language | ✅ | Editor labels/headers use t; baseline content built with reportT/reportFormatters. Correct split, verified in realRender.test.ts for both en and de |
Judgment items — rulings
(a) Signature derived from Sender's first line — ACCEPT.
sender.split('\n')[0]?.trim() ?? '', computed in buildReportContent and recomputed by applyOverrides whenever sender is overridden, rendered only when non-empty. This is the right call: the settled decision named exactly five editable fields, a sixth would exceed agreed scope, and deriving it means the signature can never drift from the sender block. Edit Sender to "Jane Doe\n99 New Address" and the signature correctly becomes "Jane Doe". Covered by realRender.test.ts:796.
(b) Mark-claimed generates no PDF — ACCEPT as vacuously satisfied.
The intent behind AC 4.5 is "no export path can bypass the user's edits." Mark-claimed is a state mutation, not an export — it did not produce or persist a PDF before this story either, and adding one would land squarely in the story's own "Out of scope" (persisting reports server-side). Since no PDF is produced, none can fail to reflect edits. The half of the AC that is observable — mark-claimed behaviour unchanged, including the #1891 excluded-lines warning — is verifiably intact.
Condition: this must be recorded as a deliberate interpretation, not left as a silently-green checkbox, so a future reader doesn't conclude the criterion was dropped.
(c) Per-field reset + edited-dot — ACCEPT.
Good affordance and the right answer to "what makes the page-level discard warning tolerable." Dot is aria-hidden with a separate accessible signal (aria-describedby hint in labelled mode, editedSuffix appended to aria-label in dense mode); reset button now meets 44×44. Two nits under Should-fix.
(d) AC 4.6 preview assertion — ACCEPT the deviation, but it must be documented.
The AC explicitly says the assertion must be that the frame renders, "not merely that a blob: URL was assigned." What openPdfPreviewModal() actually asserts is: iframe visible + src starts with blob: + the served CSP frame-src contains 'self' and blob: + zero CSP violation console messages. That is not a rendered-content check.
I'm accepting it because the POM documents both superseded attempts with CI run IDs, and the rejections are correct: Playwright's headless Chromium shell has no PDF viewer plugin so the iframe blanks without navigating (run 30530648400), and an in-page fetch(blobSrc) is blocked by connect-src 'self' — loosening which would weaken production CSP for no product reason (run 30531695763). The substituted frame-src header assertion is a genuine #1891 regression guard: it fails against the pre-fix frameSrc: ["'self'"] config, which is the actual regression class the AC was written to prevent.
Same reasoning for AC 4.3: E2E Sc.8 asserts only filename and file size, never searching the edited string in the bytes (no PDF text-extraction library in the E2E deps). The capability is genuinely proven, one layer down, in realRender.test.ts against real pdfmake/pdf-lib with real locale bundles — asserting the exact table cell position, and that the discarded baseline string is absent from the document entirely. That delegation is acceptable; assuming E2E covered it would not have been.
MUST FIX before merge
1. :global(.input) / :global(.textarea) in EditableField.module.css is dead CSS — the entire specified field treatment never applies.
client/webpack.config.cjs sets localIdentName: '[local]_[hash:base64:5]', so sharedStyles.input renders as input_aB3xY. :global(.input) compiles to a literal .input selector, which matches nothing — and there is no global (non-module) .input/.textarea rule anywhere in client/src. This is also 11 of the 13 Stylelint errors (selector-pseudo-class-no-unknown), and EditableField is the only file in the codebase using :global(.
Silently lost: the at-rest --color-bg-tertiary tint (the only idle signal that a table cell is editable, per the UX spec), border: 1px solid transparent, the hover step, the --shadow-focus-subtle/--shadow-focus split, width: 100%; box-sizing: border-box, and the dense table-cell padding. Table-cell inputs therefore fall back to the standalone .input chrome — the heavy bordered box at every cell that the UX spec explicitly rejected as noisy — and won't fill their cell.
Not a functional break (fields stay editable, labelled, and keep a visible focus ring from shared.module.css), but the shipped surface is not the approved design. Fix by applying a local class that does composes: input from '../../styles/shared.module.css'; and targeting that local class, rather than :global().
2. font-weight: 500 (EditableField.module.css:13) — direct AC 1.7 violation, flagged by declaration-property-value-disallowed-list. Use the font-weight token.
3. .visuallyHidden duplicates the existing .srOnly utility and uses the deprecated clip property (property-no-deprecated, line 126). shared.module.css:455 already provides .srOnly with the modern clip-path: inset(50%). Per the Component Reuse Policy this should reuse sharedStyles.srOnly, which resolves the Stylelint error at the same time.
4. resetFieldAriaLabel is interpolated with raw English identifiers — AC 5.1 violation.
ReportContentEditor.tsx passes { field: 'usage' } and { field: 'attachmentsNote' } at four call sites (two desktop, two mobile). In German a screen reader announces "attachmentsNote auf generierten Text zurücksetzen" — an untranslated camelCase identifier read aloud. The five cover-letter fields do this correctly by passing translated labels. Pass t('sourceReports.table.usage') and t('sourceReports.editable.attachmentsNoteLabel') instead.
Worth noting this class of bug is invisible to the current unit tests: ReportContentEditor.test.tsx uses a key-echoing t mock, which happily accepts a raw literal as an interpolation value.
In-scope findings #1904–#1908 — all genuinely fixed
| Finding | Fix verified in diff |
|---|---|
| #1904 mobile viewport shows no invoice content | ✅ ReportContentEditor.tsx now renders the .mobileCardList / .mobileCard / .mobileCardRow tree with visible labels per row, alongside the desktop table |
| #1905 reset button 24×24 below WCAG 2.5.5 | ✅ .resetButton now min-width: 44px; min-height: 44px with padding: 10px; margin: -10px to preserve layout. E2E Sc.12 measures boundingBox() ≥44 |
| #1906 silent download failure + missing i18n keys | ✅ Both parts. downloadFailed and loadingPreview added to en and de; handleDownload now routes failure through showToast('error', …), matching the Paperless pattern, so it is visible without the modal |
| #1907 report-language change bypasses discard guard | ✅ onReportLanguageChange={(lang) => guardedUpdate(() => setReportLanguageOverride(lang))} — now consistent with the other two Step-4 controls. E2E Sc.15 |
| #1908 mobile cards duplicated on desktop | ✅ Base rule .mobileCardList { display: none; } added outside the media query (line 191), flipped to flex inside @media (max-width: 767px). E2E Sc.1b asserts not visible on desktop |
Should fix (non-blocking)
- Reset icon is the Material "close" X, not the undo arrow the UX spec prescribed. An X reads as "clear this field's contents"; the action is "restore the generated text." The
aria-label/titledisambiguate it for assistive tech, but not visually.ux-designercall. Step5Actionsuses a literal⟳glyph in a bare<span>rather than the sharedSpinnerthe UX spec called for, and it isn'taria-hidden— screen readers will announce the character.realRender.test.tsleaked-key guard is too narrow:/^sourceReports\.[a-zA-Z.]+$/missessources.lines.invoiceStatus.*, which is how status labels resolve. A missing status translation would leak the raw key into the PDF undetected.expect(total.amountText).not.toContain('999999')(realRender.test.ts:367) is vacuous —amountTextis formatted currency (€999,999.00), so the bare digit run can never appear. The strong version at line 296 already carries the signal; drop or strengthen this one.- Story note now stale: the included total no longer "comes from
merge.ts" — that computation was deleted there and moved intobuildReportContent.ts, where it feeds both the total row and the cover-letter body interpolation. Better design (the two can no longer drift), but the note should be corrected so the next story doesn't look in the wrong place. - Mobile-card visible
<label>elements have nohtmlFor. The inputs are labelled viaaria-label, so AC 1.6 is met, but the visible label isn't programmatically associated with its field.
Once items 1–4 are addressed and Static Analysis is green, this has my approval — the content-model architecture is the right shape, and realRender.test.ts (real pdfmake, real pdf-lib, real en/de bundles, nothing mocked but fetch) is the strongest evidence in the PR that edits reach the exported document.
|
[product-owner] Round-2 review of PR #1909 (story #1900 — editable HTML report preview) Verdict: APPROVEDAll four MUST FIX items from my round-1 MUST FIX confirmation1. 2. Raw 3. 4.
The German announcement reads naturally because the DE string puts QA's accessible-name regression tests ( Additional changes since round 1 — reviewed, no objection
Static Analysis: Judgment call: mixed-language mobile cards — ACCEPTED, no change requiredQA flagged that a mobile card can show report-language captions ( I accept this as designed. The two label sets are different kinds of text and correctly follow different locales:
Forcing consistency means choosing one of two worse outcomes: UI-language captions (breaks WYSIWYG and reintroduces the exact mismatch the architect flagged), or report-language field labels (renders the editor unusable in an unfamiliar language). The cover-letter card already carries this property by design and I accepted it in round 1; extending the same rule to mobile cards is consistent, not a new deviation. No acceptance criterion on #1900 requires single-language cards. One follow-up I am recording rather than blocking on: report-language content is rendered without a Acceptance criteriaAll five AC groups on #1900 confirmed met, including the round-1 judgment rulings I already recorded (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 a documented deviation, since Playwright headless has no PDF viewer plugin and the E2E asserts the CSP Ready to merge from a product standpoint. |
|
[ux-designer] Round 2 — re-review of both Critical findings + Medium findings from my prior review (issuecomment-5143143659)Critical 1 — dead CSS via
|
|
[product-architect] Round-2 architecture / code-quality review of PR #1909 (story #1900). Verdict: APPROVE (posted as a comment — All five blockers from round 1 are genuinely fixed, and I verified them against the working tree rather than the stale PR diff. One Medium finding below on the mixed-label question, which I am ruling on rather than leaving open — it is a two-line change and should ride in before commit, not into refinement. Blocker verificationB1 — dead CSS. Fixed, correctly. B2 — stylelint. Fixed. B3 — the i18n contract break. Fixed structurally, which is better than the fix I asked for. Rather than threading B4 — invalid token. Fixed. B5 — untranslated identifiers in a11y labels. Fixed at all four sites ( Nits. Tests. 16 suites / 360 tests green locally across Special credit for the E2E guard at The mixed-label ruling: switch both mobile editable labels to
|
- EditableField CSS rebuilt on local composed classes (:global selectors could never match hashed CSS-module names, leaving the entire field treatment and edited-dot dead in the webpack build) - ReportContentLabels moved into ReportContent (built with reportT) and consumed by both the editor and the PDF builder — preview and export now share one report-language label source - Refund color via token class, translated reset aria-labels, read-only source-info block, discard button order, shared Spinner, modal width via Modal className, override-key builder module - E2E: computed-style regression guard for dead-CSS bugs; fixed latent strict-mode locator bug behind the shard-2 CI failure (exact: true) Co-Authored-By: Claude frontend-developer (Haiku 4.5) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com> Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) <noreply@anthropic.com>
|
🎉 This PR is included in version 2.13.0-beta.33 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.13.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
buildReportContent/applyOverrides) produces a plain-string baseline merged with user-entered overrides, replacing the previous auto-only report generation.EditableFieldshared component.Fixes #1900
Round-2 review found the composes-CSS blocker and thin E2E prose (tracked as #1904-#1908); both were fixed on this branch and those follow-up issues are already closed.
Test plan
Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com
Co-Authored-By: Claude frontend-developer (Haiku 4.5) noreply@anthropic.com
Co-Authored-By: Claude translator (Sonnet 4.5) noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) noreply@anthropic.com