feat(reports): length limits on the report wizard's editable override fields (#1941)

[steilerDev]

Summary

The step-5 override fields had no length limit anywhere — not in the editor, not on the server. A user got no signal that a field has a practical presentation limit until they saw the result in a PDF they had already sent to their bank.

• Four optional props on the shared EditableField (maxLength plus three pre-translated hint strings), so any consumer can opt in and none is forced to. Absent props keep today's unbounded behaviour exactly — the pre-existing suite passes unmodified.
• Enforcement via the native maxLength attribute, with no truncation anywhere in the value pipeline.
• The constraint is exposed two ways, not one: an always-mounted static description announced on focus, before the user is anywhere near the limit, plus a separate polite live region carrying text only while at the limit. The visible counter is aria-hidden and appears at 90% — it is a sighted-user cue, and on its own it would leave screen-reader users with nothing.

Two things that are easy to get backwards

The over-limit state is not an error. An over-limit value arrives from AI-generated cover-letter content or from derived usage text — never from the user typing past the cap, which the native attribute prevents. So it carries no aria-invalid, no error colour, no blocking, and the copy deliberately avoids "please"/"must" in both locales. AC4 requires such a value to display in full, which needs no logic at all — only the discipline not to add a defensive .slice().

editedHintId stays label-only. The ariaDescribedBy ternary was refactored into a filtered-array composition, which is a precondition of the above since a ternary cannot compose two ids. But the edited-hint gate was deliberately not extended to dense mode: dense mode already conveys edited state through the accessible name, so describing it there would announce "edited" twice. There is a dedicated regression test whose only job is to stop a future well-meaning "fix".

The limits, and why usageText is 500

Each of the seven carries its own anchor in a comment rather than a bare number — the server cap it mirrors, or the derivation bracketing it. reference 100 (invoices.invoiceNumber), subject/signature 200 (vendors.name/areas.name), sender/recipient 300, body 4000, usageText 500.

usageText is 500, not the initially suggested 120–150, because the baseline is derived, not typed: getUsageText() joins item names and budget-line descriptions each independently capped at 500 server-side, so a single budget line already admits a legal 500-character value. At 150, an ordinary invoice would load over-limit and the exception state would become the routine state — the counter would be permanent orange furniture on normal invoices, defeating its own 90% gating while nominally satisfying it. 500 is bracketed below by that floor and above by MAX_SAFE_USAGE_CHUNK_CHARS (650), leaving 150 for the derived suffix.

body is 4000 with no architect round needed: buildCoverLetterContent() emits plain flowing paragraphs with no table, no dontBreakRows and no fixed-height container, so pdfmake paginates natively and an over-long body makes more pages — it never clips. The realistic runaway is the AI path, not a verbose human.

Two premises corrected before implementation

• attachmentsNote, the field in the issue's original title, is dead code (removed in 217cb408). The issue was amended to rev 2 around the fields that actually exist. The underlying gap is real; only the example was gone.
• AC4 said "existing saved/generated value", but saved is unconstructible — an override cannot survive a reload. The two real over-limit-on-load sources are both baselines: AI-generated content and derived usageText. Tests are written against those rather than an impossible fixture.

The matching-server-bound criterion is vacuous by determination: overrides live only in the wizard reducer and never reach the server. It is recorded as such, with an explicit prohibition against building a validator for fields that never arrive.

A regression caught in review

The .metaRow wrapper was initially gated on showCounter || isEdited. Because .metaRow's margin-top stacks additively on .container's flex gap — they do not collapse — and because every call site passes maxLength while the counter only appears at 90%, the ordinary case (edited, nowhere near the cap) was rendering 12px where #1932 shipped 8px. A 50% spacing increase on normal edits, to already-approved chrome.

Gated on showCounter alone, the no-counter case is now byte-identical to pre-#1941 markup, confirmed by the ux-designer against the CSS. Reviewing their own spec, they then found the same additive-margin bug in the counter-showing case and had margin-top dropped entirely, so .container's gap is the single source of spacing throughout the component.

A pre-existing test had asserted the button was a direct child of .container and was initially updated to match the new wrapper. It has been reverted — the test was right and the production gate was wrong — and a companion test now pins the wrapped shape too, which is the assertion whose absence let the gating bug through.

Test evidence

100% statements/branches/functions/lines on both EditableField.tsx and ReportContentEditor.tsx; 158 tests passing; tsc --noEmit clean. Every negative assertion is paired with a positive control using the identical selector, so it cannot pass vacuously. jsdom was checked empirically rather than assumed: it does not clamp fireEvent.change against maxlength, so AC1 asserts attribute presence and documents the limitation instead of asserting a truncation that does not occur.

Fixes #1941

🤖 Generated with Claude Code

[steilerDev]

[product-architect]

VERDICT: APPROVE. No critical or high findings. Everything below is medium or lower and belongs in follow-ups, not in this branch. CI is fully green (Static Analysis, all 6 Jest shards, all 16 E2E shards, Docker, Trailer Check).

I verified independently rather than re-deriving the ACs (dev-team-lead owns those) or the spacing (ux-designer owns that). Four judgement calls were asked of me; I agree with three and want to correct the framing of the fourth.

---

1. Is EditableField the right home, and is the prop surface right?

Right home: yes, unambiguously. AC7 mandates it, the Component Reuse Policy demands it, and the alternative — per-call-site a11y machinery — would duplicate a static description, a live region, an id-composition, and a threshold across eight sites. Not a close call.

The no-useTranslation() convention: keep it. This is the same injection-only locale contract I enforced on reportPdf/* in #2001/PR #2007, and for the same reason. A component that calls useTranslation() pins itself to a namespace. Here the keys live under budget → sourceReports.editable, which is domain-specific to one consumer. If EditableField owned its own translation, its keys would have to move to a shared namespace and every future consumer would inherit the report wizard's phrasing whether or not it fits. The component already takes ariaLabel, editedSuffix and resetAriaLabel pre-translated; adding a fourth category of injected chrome copy is consistent, not a new tax. Do not "fix" this by giving the component a useTranslation() call.

But the prop shape is wrong, and that is the finding. The convention is fine; modelling a cohesive group as four independent optionals is not.

Nothing in the type system requires that a consumer supplying maxLength also supplies the three strings. hasMaxLength (line 60) gates the render of both srOnly spans and puts both ids into aria-describedby unconditionally (lines 65–72). A future consumer passing maxLength={200} and nothing else compiles clean, passes every existing test, and produces:

• an aria-describedby pointing at two elements that render empty — a silently degraded a11y contract, not a failing one, and
• overMaxLengthHint ?? maxLengthHint resolving to undefined in the over-limit branch (line 153).

That is the "the-prop-landed-is-not-the-prop-is-wired" family I logged on #1910/PR #2004, arriving through the type system this time instead of through a call site.

The shape that keeps the convention and removes the hazard is one optional object:

lengthLimit?: {
  max: number;
  hint: string;                 // pre-translated
  overHint?: string;            // pre-translated; falls back to hint
  reachedAnnouncement: string;  // pre-translated
};

Absent = unbounded, so AC9 becomes a single discriminant rather than four correlated optionals; present = the compiler enforces the strings. It also shrinks the call-site threading you were worried about from four props to one — the "every consumer must thread three strings to get one feature" cost largely dissolves once the group is one value built from the consumer's own namespace.

Medium, non-blocking. The defect has no reachable trigger while there is exactly one consumer that passes all four everywhere (verified: 8 of 8 call sites pass the full set). Forcing the refactor now churns eight call sites and 365 lines of tests to close a hole nothing can currently fall into. The right time is the second consumer — and that is precisely the moment the current shape would bite, so this should be a tracked follow-up rather than a note in a comment.

2. The AC5 vacuity determination — confirmed, with one sharpening

The determination holds. Verified against the code, not against the issue text:

• ReportContentOverrides appears only under client/src/** — zero occurrences in shared/src or server/src.
• No localStorage / sessionStorage anywhere under pages/ReportWizardPage/, lib/reportContent/, or components/reports/.
• wizardReducer.ts:251-256 mutates overrides in reducer state only; applyOverrides() is client-side and feeds the client-side pdfmake renderer.
• There is no enclosing <form>, so there is not even a native submission path. (This also means an over-limit baseline cannot block anything through validity.tooLong — and there is no :invalid / :user-invalid styling anywhere in client/src/styles/ or client/src/components/, so the "over-limit is not an error" design actually holds at the CSS level too, not just in the copy. That was worth checking; a single input:invalid { border-color: red } in shared styles would have quietly turned every AI-generated over-limit body into a red error field.)

The sharpening. AC5 asks about inbound validation (client → server), which is vacuous. The asymmetry that genuinely exists runs the other way: the server is the only actor that can produce an over-limit value for these fields, via GenerateReportContentResponse, and it is unbounded — LLM_MAX_TOKENS defaults to 16384 output tokens. The BODY_MAX_LENGTH comment names this as the realistic runaway itself.

I want the record to say explicitly that a server-side clamp there would be wrong, not missing. AC4 requires an over-limit baseline to render in full; truncating LLM output server-side would violate the very AC this PR implements. So the correct standing framing is: these limits are presentation guidance on typed input, not a data contract. That is also why persistence is the right and only reactivation trigger — the day a value round-trips, it becomes a data contract and all seven need real bounds. Worth adding that one clause to the issue's AC5 note so a future reader doesn't "close the gap" by clamping the producer.

3. usageText = 500 and the 650 coupling — the number is right, the comment's framing is not

The value is correct and the floor argument is sound: getUsageText() joins values each independently capped at 500 server-side, so one budget line already admits a legal 500-char baseline, and anything below 500 turns AC4's exception state into the routine state. No argument there.

The ceiling half of the comment is where I'd push back. It currently reads:

Ceiling is MAX_SAFE_USAGE_CHUNK_CHARS (650) — the shared per-chunk budget for the whole Usage cell — leaving 150 chars for the derived areaText/attachmentsNote suffix.

Three corrections, in increasing order of consequence:

(a) 650 is not a cliff. packUsageCellRows packs the cell's entire stream losslessly into as many rows as it takes. Exceeding 650 produces a continuation row, not loss. So "150 for the suffix" is a presentation preference (stay on one row), not a correctness bound. The comment reads like a budget allocation, which invites the next reader to treat 500 + 150 ≤ 650 as an invariant.

(b) The 150 is not enforced and cannot be. areaText is aggregate-unbounded (N leaf areas × 200) and attachmentsNote has no maxLength anywhere. overviewPdf.ts says exactly this in its own words — it is the stated reason the bound was moved from usageText to the whole cell. So the suffix routinely can exceed 150, and the single-row outcome is guaranteed by nothing. Which is fine; it is just not what the comment claims.

(c) The real drift risk is that 650 is no longer a constant at the point of use. Since #1973 the effective budget is usageChunkCharsForWidth(colWidths.usage) = min(650, floor(650 × usageWidth / USAGE_WIDTH_7COL)). USAGE_WIDTH_7COL is the narrowest Usage width across all 96 legal column subsets, so the ratio is ≥ 1 for every subset today and the clamp pins the result at exactly 650 — the downward branch is unreachable, by design and by that function's own doc comment. But it exists so that a future narrower Usage column can lower it. If it ever returns, say, 480, a 500-char usageText exceeds the per-chunk budget on its own, and every ordinary long usage override starts emitting a continuation row. That is the coupling worth pinning — the USAGE_TEXT_MAX_LENGTH side, not the 150.

Does #1940 eat the headroom? No — it is orthogonal. The '… ' marker is applied by buildUsageCell(packedCellRows[i], true) after packing and only for i >= 1. A cell that fits one row never gets a marker at all. So the marker's two characters can only appear in the case where the cell has already exceeded the budget — the case the packer owns and handles losslessly. It cannot consume the 150. (Its own worst case, +1 rendered line, 41 → 42 against the 44-line budget, I measured in the PR #2032 review and it is independent of this.)

So: guard test yes — but not the one the comment implies. Do not write USAGE_TEXT_MAX_LENGTH + 150 <= MAX_SAFE_USAGE_CHUNK_CHARS; that asserts a false invariant and would be a test that pins a fiction. Write the one that is true and load-bearing:

expect(USAGE_TEXT_MAX_LENGTH).toBeLessThan(usageChunkCharsForWidth(USAGE_WIDTH_7COL));

— the input cap must stay strictly below the narrowest computed per-chunk budget, so a value typed to the cap always fits one row on its own. That survives #1973's computed-width era, and it fails loudly the day someone narrows the Usage column or lowers 650.

This belongs in #1950 ("Guard test for MAX_SAFE_USAGE_CHUNK_CHARS ceiling drift"), which is still open in this batch — one guard, one owner, rather than a bespoke assertion here. #1950's spec should be extended to cover the input-cap side, which needs USAGE_TEXT_MAX_LENGTH exported from ReportContentEditor.tsx (it is module-private today). Keep the constant in the editor and let the renderer's test import it — an input constraint living in the renderer would invert the dependency.

I would also reword the comment's ceiling clause to say what is actually true: "the cell's per-chunk render budget is 650 (computed, clamped); exceeding it costs a continuation row, never content. 500 is chosen to stay clear of it so a typed value never needs one on its own — the derived suffix is not bounded by input and the packer is what keeps the cell correct." Low priority, but AC2 asks for the anchor to be recorded, and a mis-stated anchor is worse than a terse one.

4. Wiki / ADR — agreed, with one addition to the already-open ADR-034 pass

No new wiki page, no new ADR, no Schema or API-Contract change. No endpoint, no table, no column, no shared type; shared/ and server/ are untouched. This is an editor input constraint upstream of everything ADR-034 governs, and the PR correctly honoured the scope guard by not touching the renderer's chunk ceilings.

But one sentence does belong in the ADR-034 pass I already owe (currently carrying: the stale 69.28/33.54/266.16pt figures and the "every cell" vs Usage-only test-coverage gap from #2008; invariant 1's under-claim and the Formatters / AppFormatters extends Formatters one-liner from #2028; and line 152's call-site quote stale a third time plus line 148's now-under-satisfied "bound what a cell renders" rule from #2032).

This PR adds a fifth item, and it is a genuine one. ADR-034 line 148's rule — bound what a cell renders — has just acquired an input-side counterpart: the editor caps typed contributions to the Usage cell at 500 against a computed 650 render budget. That is a cross-module coupling between the editor and the renderer, and it is currently documented only in a code comment inside a client component. Wrong altitude. One sentence in ADR-034 — "input caps are set below the render budget so a typed value never needs a continuation row on its own; the render budget remains the only correctness bound" — puts it where the next person changing 650 will actually read it, and it also states the (a)/(b) correction above at the level where it matters. I will fold it into that pass.

5. The flex-gap / margin mechanism — yes, record it, and I'll say where

I agree this earned durability. It bit in the production code and then again in the spec written to review the production code — that is the signature of a mechanism people reason about wrongly, not a slip.

The right home is not ADR-034 (PDF pipeline) and not the Architecture page. It is the Style Guide, as a spacing-model rule, because it is a fact about how this design system composes: a container that owns its spacing via gap must not have children that add their own margin in the gap axis — flex gap and child margin are additive, not collapsing. That is a one-line rule with real teeth, it generalises past EditableField, and it is exactly the class of thing a style guide exists to stop being rediscovered. I don't own Style-Guide.md, so I'm recommending it to @UX-Designer rather than writing it.

I'm recording it in my own memory too — it is the same shape as other traps where two mechanisms both fire, nobody notices, and the only symptom is that the result looks slightly wrong.

---

Remaining findings (all low / non-blocking)

L1 — CSS duplication. .counter and .counterOverLimit repeat flex, text-align and font-size verbatim; only color differs. This file already uses CSS Modules composes (.field composes: input from shared), so .counterOverLimit { composes: counter; color: … } is the idiomatic form and removes three lines that can drift apart.

L2 — line-numbered cross-references will rot. The new comments added eight of them. I verified all eight today: invoices.ts:25 ✓, vendors.ts:35 ✓, areas.ts:12 ✓, invoiceBudgetLines.ts:49 ✓, overviewPdf.ts:245 ✓, coverLetterPdf.ts:59-74 ✓, coverLetterPdf.ts:76-81 ✓ — and buildReportContent.ts:53, which is off by one (getUsageText is declared at line 52). Trivial in itself, but this is the exact family that has now gone stale on me three separate times on ADR-034's line 152 (#1939, #2008, #2032). Since AC2 requires the anchor to be recorded, and a rotted line number is a worse anchor than none, the durable form is the symbol — invoices.ts createInvoiceSchema.invoiceNumber — not the line. Worth converting whenever these comments are next touched; not worth a round now.

L3 — redundant non-null assertions. maxLength! on lines 61 and 63 is unnecessary: hasMaxLength is a const boolean over a never-reassigned binding, so TypeScript's aliased-condition narrowing already applies. Cosmetic.

---

What I verified

• AC4 / no truncation: zero .slice(0, .substring, .substr( in EditableField.tsx, ReportContentEditor.tsx, or applyOverrides.ts. The value pipeline is clean.
• All eight call sites pass the full four-prop set (grep -c "maxLength=" ReportContentEditor.tsx → 8, matching seven fields with usageText rendered at both the dense-cell and mobile-card sites).
• The .metaRow gating regression is now pinned from both directions: the reverted pre-existing test asserts the direct-child shape in the no-counter case, and the new companion test asserts the wrapped shape in the counter case. Reverting the test rather than updating it to match the new markup was the right call — the test was correct and the production gate was wrong, and the companion test is precisely the assertion whose absence let the bug through. That is the pattern I want to see more of.
• The aria-describedby id-count coverage (0/1/2/3) is explicit, and the dense-mode editedHintId regression guard does what it says. The composition refactor from a ternary to a filtered array was a genuine precondition, not incidental churn.
• No wiki submodule change in this PR, so no ref-vs-remote check applies.

[github-actions]

🎉 This PR is included in version 2.14.0-beta.21 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀