From d3811f2245a2d804c79d1bb956945d737ef15561 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 05:38:01 +0200 Subject: [PATCH 1/2] feat(reports): merge runt continuation chunks and mark continuation rows A continuation row blanks every leading column, so under a freshly repeated header it is visually indistinguishable from the orphaned-cell bug #1929 fixed. The degenerate form is worse: with no minimum trailing-chunk size, text whose tail lands just past a chunk boundary produced a row blank except for a single stray character, which reads as document corruption on a document whose whole purpose is to look credible. Both forms were reproduced by rasterized render. - Add packUsageCellRowsWithMinimum, which merges a would-be runt into the row before it. It wraps packUsageCellRows' row-level output rather than changing the chunker, because a runt arises from two paths -- the hard-split remainder and the "fits a fresh row" branch -- and a fix inside splitIntoPageSafeChunks would miss the second. Both of those functions are byte-identical to beta. - Resolve AC1's tension with AC2 by gating the reduced-budget repack behind an actual runt check. Packing once at full budget and returning it untouched when no runt exists preserves today's exact row counts for everything that fits one row and for multi-row content that already divides cleanly; an unconditional budget reduction would have split content that fits today, regressing the real-render-verified zero-degradation range for no reason. No ceiling is re-derived, and usageChunkCharsForWidth's one-sided clamp is untouched. - Prepend a literal ellipsis run to continuation rows. The ux-designer chose ink shape over colour or fill deliberately: this is a bank document that gets scanned, and colour is what degrades under greyscale printing. Usage is also the only cell that can carry the signal, since every other cell is blanked on a continuation row and #1973's subsets may leave no leading column at all. The marker is render-time only and never enters the reconstruction invariant. - Express the floor per-subset as max(MIN_CONTINUATION_ROW_FLOOR_CHARS, usageSafeTokenChars) so a merged runt fills at least one real line at whatever width the active subset gives Usage. Adds fontkit as an exact-pinned devDependency to verify the font has a real glyph for U+2026 rather than .notdef. It was already a production transitive dependency via pdfmake -> pdfkit, so this installs nothing new and leaves the runtime image unchanged. Fixes #1940 Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude ux-designer --- .../qa-integration-tester/MEMORY.md | 1 + ...ory-1940-continuation-marker-runt-merge.md | 144 ++++++ client/package.json | 2 + client/src/lib/reportPdf/overviewPdf.test.ts | 450 ++++++++++++++++-- client/src/lib/reportPdf/overviewPdf.ts | 126 ++++- client/src/lib/reportPdf/realRender.test.ts | 257 +++++++++- package-lock.json | 22 +- 7 files changed, 945 insertions(+), 57 deletions(-) create mode 100644 .claude/agent-memory/qa-integration-tester/story-1940-continuation-marker-runt-merge.md diff --git a/.claude/agent-memory/qa-integration-tester/MEMORY.md b/.claude/agent-memory/qa-integration-tester/MEMORY.md index ca2997a52..cb5f20862 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) +- [Issue #1940 — continuation marker + runt-merge](story-1940-continuation-marker-runt-merge.md) (2026-08-06) — `packUsageCellRowsWithMinimum`/`… ` marker; centralized `stripContinuationMarker` pattern (both test files); ripple far wider than the 2 named tests (whole "cell-scope invariant" block's shared helper in realRender.test.ts); fontkit glyph-id-0 technique (added as pinned devDependency); blank pdfmake cells DO get `.positions`; mid-list-runt fixture recipe; git-revert genuine-regression proof (clean import-error + 2 real assertion failures). - [Issue #1912 — ESM mock blast radius](issue-1912-esm-mock-blast-radius.md) (2026-08-06) — new export on a shared module breaks a *different* file's partial `jest.unstable_mockModule` factory, not the one I checked; mechanical sweep + batch-jest-run pattern - [Issue #1911 — splitKind field](story-1911-splitkind.md) (2026-08-05) — server UNION `origin`-column trap (AC1.5: "≠S per arm" not "≥2 distinct per arm"); UNION-dedup regression guard (AC1.9); `jest.spyOn(db,'all')` pass-through + drizzle `queryChunks` introspection for round-trip proof; client-side ripple broke 9 pre-existing tests in 2 files beyond the "add splitKind default" checklist item (found by just running the file); AC1.2 git-stash anti-vacuity proof (9/70 genuinely fail on reverted code). - [Issue #2001 — remove TFunction from reportPdf](story-2001-remove-tfunc-reportpdf.md) (2026-08-05, 2 rounds) — 7 test files total; realRender.test.ts has renderOverviewPdfContent (KEEP its t) vs buildOverviewContent/generateReportPdf (REMOVE); perl regex targets `{ attachDocuments: }` context to avoid removing renderOverviewPdfContent args; comment on same line blocks regex → explicit Edit; 218 tests pass round 2. diff --git a/.claude/agent-memory/qa-integration-tester/story-1940-continuation-marker-runt-merge.md b/.claude/agent-memory/qa-integration-tester/story-1940-continuation-marker-runt-merge.md new file mode 100644 index 000000000..cd3e11de3 --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/story-1940-continuation-marker-runt-merge.md @@ -0,0 +1,144 @@ +--- +name: story-1940-continuation-marker-runt-merge +description: Issue #1940 (continuation-row "… " marker + runt-remainder merge in overviewPdf.ts) — centralized marker-stripping pattern, fontkit glyph-coverage technique, ripple scope +metadata: + type: project +--- + +Issue #1940: `packUsageCellRowsWithMinimum` (AC1/AC2 runt-remainder merge) and `buildUsageCell`'s +`isContinuation` marker (AC5, a literal `{ text: '… ' }` run prepended to every Usage continuation +row, no color override). Tests: `client/src/lib/reportPdf/overviewPdf.test.ts` (+~350 lines: new +`packUsageCellRowsWithMinimum` describe block, `AC5`/`AC8`/`AC2` describe blocks) and +`client/src/lib/reportPdf/realRender.test.ts` (+~190 lines: new `#1940` describe block). + +## The marker-stripping ripple was much larger than the two "extend this block" instructions implied + +The spec named 2 existing tests to fix by hand (scenario 10/11 in overviewPdf.test.ts). Actually +affected once the marker landed: **every existing test that reconstructs a Usage cell's text on a +row known to be a continuation row**, in BOTH files. Found by grepping every `splitUsageCell(`/ +`rowTexts(`/`usageCellText(` call site and checking whether the row it read was ever index >= 1 of +a packed group: +- `overviewPdf.test.ts`: scenario 10, scenario 11, and the "[#1959 fix round] suffix gets a row of + its own" test (3 existing tests). +- `realRender.test.ts`: scenario 19c's reconstruction (`usageCellText` used directly, not + `splitUsageCell`), AND — the big one — the entire "cell-scope invariant" describe block's shared + `renderCellScopeRow` helper, which ~8 downstream tests depend on (700+400, 700+20-leaf-area, + 2000-alone, the three page-count-saturation guards, two #1968 regressions). That helper also + derived its expected row COUNT from bare `packUsageCellRows`, which could disagree with + production's `packUsageCellRowsWithMinimum` count whenever a runt merge fires on one of those + large fixtures — fixed by switching the helper to call `packUsageCellRowsWithMinimum` with the + same `Math.max(20, USAGE_SAFE_TOKEN_CHARS_6COL)` floor production uses (that block's fixture is + always the 6-col/claim shape). + +**Lesson**: when a spec says "extend the existing X and Y tests," always grep every call site of +the shared reconstruction helper across BOTH files before assuming the ripple is scoped to the two +named tests — a shared test helper used by N tests is one bug away from breaking all N silently. + +## Centralization pattern (applied identically in both files) + +Added `stripContinuationMarker(runs): T[]` once per file — strips exactly +one leading run whose `.text === '… '` AND `.color === undefined`. Gated behind an explicit +`opts.isContinuation` boolean on `rowTexts`/`splitUsageCell`, never applied unconditionally by +substring match (that would risk masking a genuine defect that happened to produce a leading '… ' +in real content on row 0, which never gets the marker). Every call site on a continuation row +either passes `{ isContinuation: true }` directly, or (when mapping over a whole row group) +`{ isContinuation: i > 0 }` since row 0 of a packed group is never a continuation row. + +## Deriving expected row counts: use `packUsageCellRowsWithMinimum`, not `packUsageCellRows`/`splitIntoPageSafeChunks` + +Any test that derives "how many rows should this produce" from the bare packer instead of the +wrapper is a latent bug the moment a runt merge fires on that fixture — happened to not matter for +the specific pre-existing fixtures in this PR (verified by tracing: none of them contained an +actual runt), but the cell-scope block's fixtures easily could have. Always derive from the SAME +formula `buildOverviewContent` calls: `packUsageCellRowsWithMinimum(segments, usageChunkChars, +Math.max(20, usageSafeTokenCharsForWidth(colWidths.usage)))`. For the two reference shapes, +`usageChunkChars` is always `MAX_SAFE_USAGE_CHUNK_CHARS` (650) — `usageChunkCharsForWidth`'s +one-sided clamp means the 6-col shape's wider Usage column still clamps down to 650, never up. + +## Pathological-runt fixture that reliably produces a MID-list runt (not just trailing) + +`'A'.repeat(BUDGET) + ' hi ' + 'B'.repeat(18)` at a small BUDGET — the leftover `' hi '` (4 chars) +doesn't fit after `'A'*BUDGET` fills row 0, but DOES fit its own row, so plain `packUsageCellRows` +strands it alone as row 1, sandwiched before `'B'*18`'s own row 2. Useful whenever a spec explicitly +wants "not merely a trailing runt" coverage — most naive fixtures (a single oversized token) only +ever produce a TRAILING runt. + +## fontkit glyph-coverage technique (AC9's "verify the glyph exists, don't assume from precedent") + +`fontkit` is a transitive dependency (via `pdfmake` -> `pdfkit`) already resolvable at +`node_modules/fontkit` even before being declared — added it + `@types/fontkit` as EXACT-pinned +`client/package.json` devDependencies (both `2.0.4`, matching the already-hoisted version) per the +Dependency Policy, then `npm install` (root) to regenerate the lockfile — `npm ls fontkit`/`npm ls +undici` afterward confirmed no tree damage; audit findings unchanged (all pre-existing, unrelated to +fontkit). Technique, verified working under this repo's ts-jest ESM jsdom config with zero jest +config changes: + +```ts +const fontkitModule = await import('fontkit'); +const fontkit = (fontkitModule as { default?: typeof fontkitModule }).default ?? fontkitModule; +const vfsModule = await import('pdfmake/build/vfs_fonts'); // same module loader.ts uses +const vfs = (vfsModule as { default?: Record }).default ?? vfsModule; +const font = fontkit.create(Buffer.from(vfs['Roboto-Regular.ttf'], 'base64')); +const glyph = font.glyphForCodePoint(0x2026); // U+2026 HORIZONTAL ELLIPSIS +expect(glyph.id).not.toBe(0); // glyph id 0 == .notdef by spec — the real "tofu box" signal +``` + +Confirmed empirically (Roboto-Regular): U+2026 -> glyph id 400 (real). A genuinely-unmapped astral +codepoint (`0x10ffff`) -> glyph id 0 — a positive control proving glyph id 0 is reachable/meaningful +in this exact font, not a fontkit quirk that always returns non-zero. **Glyph id, not a raw +advance-width measurement, is the dispositive check** — `.notdef` commonly has a real non-zero +advance in real fonts, so width alone can't rule out a tofu box. + +## Blank pdfmake cells DO get `.positions` populated (verified empirically, not assumed) + +Before writing an AC7 "every cell in the row shares one pageNumber" assertion across a continuation +row (which has blank leading/amount cells from `buildEmptyBodyCell`), I worried `cellPageNumber` +(which throws on an empty `.positions` array) would fail on those blank cells. Verified via a +disposable probe test: an empty-string `{text:'', style:'tableCell'}` cell gets exactly ONE +`.positions` entry after a real render, same shape as a non-empty cell's first entry. So +`row.map(cellPageNumber)` is safe and meaningful across a full row including blanks — no need to +filter to non-empty cells or weaken the assertion. + +## Genuine-regression proof + +Backup/restore technique (per [story-1929-round2-real-render-technique.md](story-1929-round2-real-render-technique.md)): +reverted `overviewPdf.ts` to `git show HEAD:...` (pre-#1940), ran the new test surface: +- `overviewPdf.test.ts`: whole suite fails to even LOAD (`SyntaxError: ... does not provide an + export named 'packUsageCellRowsWithMinimum'`) — a clean, unambiguous module-resolution proof for + the entire new Part A/B surface at once. +- `realRender.test.ts`'s new `#1940` describe block: 2 of 4 tests fail with REAL assertion + failures (not import errors, since the module still loads fine using only pre-existing exports) + — the AC9 runt-avoidance test fails because the last row's marker-stripped usage text is too + short, and the AC5 marker test fails because the first run is real prose (`'zzz...'`), not + `{text:'… '}`. The other 2 (glyph coverage, `_minWidth`/page-number) correctly still pass + pre-fix, since they test generic layout properties unrelated to this fix — expected, not a gap. + +Restored via `cp` from a `/tmp` backup; confirmed `git diff --stat` on `overviewPdf.ts` matched the +pre-revert diff exactly (99 insertions / 5 deletions) before finishing. + +## Review round 1 fixes (product-architect + security-engineer) + +- **`fontkit` devDependency confirmed harmless**: security-engineer verified it was already a + production transitive dependency via `pdfmake` -> `pdfkit`, so declaring it in + `client/package.json` installs nothing new — the Docker runtime image is byte-identical. +- **`TS2352` — Jest passing does NOT mean `tsc` is clean.** `(fontkitModule as { default?: + typeof fontkitModule }).default ?? fontkitModule` compiled fine under `ts-jest` (which runs no + type diagnostics by default) but failed `npx tsc --noEmit -p client/tsconfig.json` — "neither + type sufficiently overlaps with the other." Fix: route through `unknown` first, exactly like the + `vfsModule` cast two lines below it already did: `(fontkitModule as unknown as { default?: + typeof fontkitModule }).default ?? fontkitModule`. **Lesson reinforced a third time in this + batch** (after #1911's missing factory field, #1912's ESM mock): run the scoped `npx tsc --noEmit + -p client/tsconfig.json` (or the server equivalent) before handback whenever a test file adds a + nontrivial type assertion — Jest green is not proof the build is green. Make this reflexive, not + something that has to be asked for. +- **Bare literal `20` retyped at 6 call sites — exactly the anti-pattern issue #1950 exists to + prevent.** Once `frontend-developer` extracted `MIN_CONTINUATION_ROW_FLOOR_CHARS` (exported from + `overviewPdf.ts`, value `20`), all 6 sites (4 in `overviewPdf.test.ts`, 2 in `realRender.test.ts`) + were switched from `Math.max(20, ...)` to `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, ...)`. + **The two operands are a deliberate non-redundant pair, not simplifiable to either alone**: + `usageSafeTokenCharsForWidth(usageWidth)` does the main per-subset-width work; the named constant + is the absolute fallback floor for subsets where that per-line figure is itself small. Added a + comment at the point of use so a future reader doesn't "simplify" one operand away. +- **General lesson**: whenever a spec/AC references a bare numeric literal that also appears as a + production magic number, check whether a name for it exists (or is about to land) before writing + the test — retyping it in N places is itself the bug class this batch's #1950 targets. diff --git a/client/package.json b/client/package.json index f6393e2ad..c21f84c55 100644 --- a/client/package.json +++ b/client/package.json @@ -26,6 +26,7 @@ "@babel/core": "7.29.7", "@babel/preset-react": "7.29.7", "@babel/preset-typescript": "7.29.7", + "@types/fontkit": "2.0.4", "@types/pdfmake": "0.3.3", "@types/react": "19.2.18", "@types/react-dom": "19.2.4", @@ -33,6 +34,7 @@ "copy-webpack-plugin": "14.0.0", "css-loader": "7.1.4", "css-minimizer-webpack-plugin": "8.0.0", + "fontkit": "2.0.4", "html-webpack-plugin": "5.6.8", "mini-css-extract-plugin": "2.10.2", "style-loader": "4.0.0", diff --git a/client/src/lib/reportPdf/overviewPdf.test.ts b/client/src/lib/reportPdf/overviewPdf.test.ts index f2c18e26c..3792feb91 100644 --- a/client/src/lib/reportPdf/overviewPdf.test.ts +++ b/client/src/lib/reportPdf/overviewPdf.test.ts @@ -55,6 +55,7 @@ import { buildOverviewContent, splitIntoPageSafeChunks, packUsageCellRows, + packUsageCellRowsWithMinimum, buildUsageTextRuns, computeColumnWidths, usageSafeTokenCharsForWidth, @@ -65,6 +66,7 @@ import { USAGE_SAFE_TOKEN_CHARS_6COL, VENDOR_SAFE_TOKEN_CHARS, MAX_SAFE_USAGE_CHUNK_CHARS, + MIN_CONTINUATION_ROW_FLOOR_CHARS, } from './overviewPdf.js'; import { tableOffsetsTotal, printableWidth } from './pageGeometry.js'; @@ -142,16 +144,44 @@ function makeContent(overrides: Partial = {}): ReportContent { }; } +// #1940 AC5: buildUsageCell prepends a single literal `{ text: '… ' }` run (U+2026 + space, no +// `color` override) as the very first run of a CONTINUATION row's (packedCellRows index >= 1) +// Usage cell — the visual "this row continues" signal. Every reconstruction helper in this file +// concatenates a cell's ENTIRE run array, so without stripping, every continuation-row assertion +// would silently start comparing `'… ' + text` against `text`. Centralized HERE, once, rather than +// patched ad hoc per call site: strips exactly one leading run whose `.text === '… '` and which +// carries no `color`. Only ever applied when the CALLER already asserts the row is a continuation +// row (`isContinuation: true`) — never applied unconditionally by substring/prefix match, which +// would risk silently masking a genuine reconstruction defect that happened to produce a leading +// '… ' in real user content on a row that was never supposed to carry the marker. +function stripContinuationMarker(runs: T[]): T[] { + if (runs.length > 0 && runs[0]!.text === '… ' && runs[0]!.color === undefined) { + return runs.slice(1); + } + return runs; +} + // Flattens a pdfmake `table.body` row into plain text strings for easy assertions. Cells that are // `stack`s (the Usage column when an attachment note or area text is present) yield `undefined`. // The allocated-amount cell's `text` is always an array of runs (story #1923: the isDeposit inline // label is a distinct, separately-styled run) — concatenate those runs' own `.text` values so // existing plain-string assertions keep working; dedicated tests inspect the raw run array instead // where the per-run styling itself is under test. -function rowTexts(row: unknown): (string | undefined)[] { - return (row as { text?: string | { text: string }[] }[]).map((cell) => { +// +// `opts.isContinuation` (#1940): pass `true` when `row` is known to be a Usage continuation row — +// strips the leading '… ' marker (see stripContinuationMarker above) from the LAST cell before +// reconstructing, since the Usage cell is always the last cell in a row (buildOverviewContent +// always appends it last) and the only cell that is ever a run array on a continuation row (every +// other column is blanked to a plain '' string by buildEmptyBodyCell). +function rowTexts(row: unknown, opts: { isContinuation?: boolean } = {}): (string | undefined)[] { + const cells = row as { text?: string | { text: string; color?: string }[] }[]; + return cells.map((cell, i) => { if (Array.isArray(cell.text)) { - return cell.text.map((run) => run.text).join(''); + const runs = + opts.isContinuation && i === cells.length - 1 + ? stripContinuationMarker(cell.text) + : cell.text; + return runs.map((run) => run.text).join(''); } return cell.text; }); @@ -184,16 +214,24 @@ function usageRunsText(text: unknown): string { // silently drifts with the fixture's word count. const GREY = '#6b7280'; -function splitUsageCell(cell: unknown): { +// `opts.isContinuation` (#1940): pass `true` when `cell` is known to be a Usage continuation row's +// cell — strips the leading '… ' marker (see stripContinuationMarker above) before parsing, so the +// marker never leaks into `usageText` (it would otherwise land in the non-grey prefix, ahead of +// any grey meta run). +function splitUsageCell( + cell: unknown, + opts: { isContinuation?: boolean } = {}, +): { usageText: string; metaRun: { text: string; color?: string } | null; /** Raw grey runs preserving all pdfmake run properties including wordBreak. */ greyRuns: { text: string; color?: string; wordBreak?: string }[]; } { - const runs = (cell as { text: { text: string; color?: string; wordBreak?: string }[] }).text; - if (!Array.isArray(runs)) { + const rawRuns = (cell as { text: { text: string; color?: string; wordBreak?: string }[] }).text; + if (!Array.isArray(rawRuns)) { throw new Error('Usage cell .text is not a run array — buildUsageTextRuns wiring changed?'); } + const runs = opts.isContinuation ? stripContinuationMarker(rawRuns) : rawRuns; const greyIndexes = runs.map((run, i) => (run.color === GREY ? i : -1)).filter((i) => i !== -1); if (greyIndexes.length === 0) { return { usageText: runs.map((r) => r.text).join(''), metaRun: null, greyRuns: [] }; @@ -653,6 +691,174 @@ describe('packUsageCellRows (#1959 fix round: one page-safe budget for the whole }); }); +// ─── packUsageCellRowsWithMinimum (#1940 AC1/AC2: runt-remainder merge) ─────────────────────── +// +// Wraps packUsageCellRows with a floor on every CONTINUATION row's (index >= 1) character count, +// so a would-be runt remainder merges into the row before it instead of rendering as a near-empty +// row indistinguishable from a broken document (#1940 — the "Could Have" deferred from #1929, +// observed twice in a real rendered PDF). Every assertion below is expressed against the INPUT/ +// budget/floor, or against a direct packUsageCellRows() call for comparison — never against the +// wrapped function's own prior output — same discipline as the packUsageCellRows block above. +describe('packUsageCellRowsWithMinimum (#1940 AC1/AC2: runt-remainder merge)', () => { + const BUDGET = 20; // same small budget as the packUsageCellRows block, for legible assertions + const MIN = 5; + + function flatten(rows: UsageCellSegment[][]): UsageCellSegment[] { + return rows.flat(); + } + function totalText(rows: UsageCellSegment[][]): string { + return flatten(rows) + .map((s) => s.text) + .join(''); + } + function rowLengths(rows: UsageCellSegment[][]): number[] { + return rows.map((row) => row.reduce((sum, s) => sum + s.text.length, 0)); + } + // Local word-boundary-clean filler, scoped to this describe block per this file's own + // convention (see proseOfLength further down, duplicated rather than hoisted-and-shared). + function fillerOfLength(exactLength: number): string { + const words = ['lorem', 'ipsum', 'dolor', 'sit', 'amet', 'consectetur', 'adipiscing']; + let text = ''; + let i = 0; + for (;;) { + const word = words[i % words.length]!; + const candidate = text.length === 0 ? word : `${text} ${word}`; + if (candidate.length >= exactLength) return candidate.slice(0, exactLength); + text = candidate; + i++; + } + } + + it('minTrailingChars <= 0 degrades to a direct packUsageCellRows call — no floor applies', () => { + const segments: UsageCellSegment[] = [ + { text: 'aaaa bbbb cccc ddddd' }, + { text: '\nEEEEE', meta: true }, + ]; + expect(packUsageCellRowsWithMinimum(segments, BUDGET, 0)).toEqual( + packUsageCellRows(segments, BUDGET), + ); + expect(packUsageCellRowsWithMinimum(segments, BUDGET, -3)).toEqual( + packUsageCellRows(segments, BUDGET), + ); + }); + + it('minTrailingChars >= maxChars degrades to a direct packUsageCellRows call — a floor that would consume the whole budget is not a floor', () => { + const segments: UsageCellSegment[] = [ + { text: 'aaaa bbbb cccc ddddd' }, + { text: '\nEEEEE', meta: true }, + ]; + expect(packUsageCellRowsWithMinimum(segments, BUDGET, BUDGET)).toEqual( + packUsageCellRows(segments, BUDGET), + ); + expect(packUsageCellRowsWithMinimum(segments, BUDGET, BUDGET + 10)).toEqual( + packUsageCellRows(segments, BUDGET), + ); + }); + + it("propagates packUsageCellRows's non-positive-maxChars throw rather than the degenerate-minTrailingChars guard swallowing it", () => { + const segments: UsageCellSegment[] = [{ text: 'x' }]; + expect(() => packUsageCellRowsWithMinimum(segments, 0, MIN)).toThrow( + 'packUsageCellRows: maxChars must be positive, got 0', + ); + expect(() => packUsageCellRowsWithMinimum(segments, -1, MIN)).toThrow( + 'packUsageCellRows: maxChars must be positive, got -1', + ); + }); + + it('fast-path preservation, single row: content of exactly maxChars returns EXACTLY what packUsageCellRows returns (deep equality, not merely "one row")', () => { + const segments: UsageCellSegment[] = [{ text: 'a'.repeat(BUDGET) }]; + expect(packUsageCellRowsWithMinimum(segments, BUDGET, MIN)).toEqual( + packUsageCellRows(segments, BUDGET), + ); + }); + + it('fast-path preservation, multi-row no-runt: two healthy rows come back unchanged — proves the fix path never engages when unneeded', () => { + const segments: UsageCellSegment[] = [ + { text: 'aaaa bbbb cccc ddddd' }, // exactly BUDGET (20 chars): one full row + { text: '\nEEEEE', meta: true }, // 6 chars, >= MIN(5): not a runt + ]; + const raw = packUsageCellRows(segments, BUDGET); + expect(raw).toHaveLength(2); + expect(rowLengths(raw)[1]).toBeGreaterThanOrEqual(MIN); // sanity: this fixture has no runt + expect(packUsageCellRowsWithMinimum(segments, BUDGET, MIN)).toEqual(raw); + }); + + it("the pathological single-token case: a token of length 2*maxChars + 1 hard-splits to [maxChars, maxChars, 1] under plain packing (today's runt) — the wrapped function leaves no row after the first under MIN, and every row stays <= maxChars", () => { + const token = 'z'.repeat(BUDGET * 2 + 1); // 41 chars + const segments: UsageCellSegment[] = [{ text: token }]; + + // Document the exact pre-fix pathology this AC exists to close. + const raw = packUsageCellRows(segments, BUDGET); + expect(rowLengths(raw)).toEqual([BUDGET, BUDGET, 1]); + + const rows = packUsageCellRowsWithMinimum(segments, BUDGET, MIN); + for (const length of rowLengths(rows).slice(1)) { + expect(length).toBeGreaterThanOrEqual(MIN); + } + for (const length of rowLengths(rows)) { + expect(length).toBeLessThanOrEqual(BUDGET); + } + expect(totalText(rows)).toBe(token); // I1 still holds + }); + + it('a MID-list runt (not merely trailing) is also eliminated: a short word stranded alone between two full-width rows', () => { + // 'A'x20 fills row 0 exactly; the leftover ' hi ' (4 chars, < MIN) doesn't fit alongside the + // next token ('B'x18, itself <= BUDGET) but DOES fit a row of its own, so plain packing stands + // it up as its own near-empty row — sandwiched between two full rows, not at the tail. + const text = 'A'.repeat(BUDGET) + ' hi ' + 'B'.repeat(18); + const segments: UsageCellSegment[] = [{ text }]; + + const raw = packUsageCellRows(segments, BUDGET); + expect(raw).toHaveLength(3); + const rawLengths = rowLengths(raw); + expect(rawLengths[1]).toBeLessThan(MIN); // confirms the runt sits at index 1 + expect(raw.length - 1).toBeGreaterThan(1); // 1 < rows.length - 1: genuinely mid-list, not trailing + + const rows = packUsageCellRowsWithMinimum(segments, BUDGET, MIN); + for (const length of rowLengths(rows).slice(1)) { + expect(length).toBeGreaterThanOrEqual(MIN); + } + for (const length of rowLengths(rows)) { + expect(length).toBeLessThanOrEqual(BUDGET); + } + expect(totalText(rows)).toBe(text); + }); + + describe('I1: reconstruction holds across every fixture above, plus one carrying a meta segment', () => { + it.each<[string, UsageCellSegment[]]>([ + ['exact-fit single row', [{ text: 'a'.repeat(BUDGET) }]], + ['multi-row no-runt', [{ text: 'aaaa bbbb cccc ddddd' }, { text: '\nEEEEE', meta: true }]], + ['pathological hard-split token', [{ text: 'z'.repeat(BUDGET * 2 + 1) }]], + ['mid-list runt', [{ text: 'A'.repeat(BUDGET) + ' hi ' + 'B'.repeat(18) }]], + [ + 'mid-list runt shape with a trailing meta segment', + [ + { text: 'A'.repeat(BUDGET) + ' hi ' + 'B'.repeat(18) }, + { text: '\nGround Floor', meta: true }, + ], + ], + ])('%s', (_label, segments) => { + const expected = segments.map((s) => s.text).join(''); + const rows = packUsageCellRowsWithMinimum(segments, BUDGET, MIN); + expect(totalText(rows)).toBe(expected); + }); + }); + + it('property-style sweep: for total lengths stepping from maxChars - minTrailingChars up to maxChars * 4, NO row after the first is EVER < minTrailingChars, and NO row EVER exceeds maxChars', () => { + for (let len = BUDGET - MIN; len <= BUDGET * 4; len++) { + const text = fillerOfLength(len); + const segments: UsageCellSegment[] = [{ text }]; + const rows = packUsageCellRowsWithMinimum(segments, BUDGET, MIN); + for (const length of rowLengths(rows).slice(1)) { + expect(length).toBeGreaterThanOrEqual(MIN); + } + for (const length of rowLengths(rows)) { + expect(length).toBeLessThanOrEqual(BUDGET); + } + } + }); +}); + describe('buildUsageTextRuns (#1929 round 2/3/4 word-break follow-up findings, scenarios 1/2/3)', () => { // Pin the actual threshold constants rather than re-typing 16/22 — if pageGeometry.ts's // per-char estimate or the USAGE_WIDTH_*COL values ever change, this test's own expectations @@ -1258,12 +1464,17 @@ describe('buildOverviewContent — Usage chunking into continuation rows (scenar const result = buildOverviewContent(content, new Map()); const table = getTable(result); - // Derive the expected chunk count from splitIntoPageSafeChunks itself (unit-tested separately - // above) rather than a naive Math.ceil(len/maxChars) — the algorithm greedily fills each chunk - // up to the last clean word boundary <= maxChars, so it can produce one or two MORE chunks - // than the arithmetic minimum. This test's job is to verify buildOverviewContent WIRES that - // chunking output into the right row shapes, not to re-derive the chunking algorithm's output. - const expectedChunks = splitIntoPageSafeChunks(usageText, MAX_SAFE_USAGE_CHUNK_CHARS).length; + // Derive the expected chunk count from the SAME formula buildOverviewContent itself calls + // (#1940 AC1: packUsageCellRowsWithMinimum, not the bare splitIntoPageSafeChunks a runt-merge + // fix can change the row count of) rather than a naive Math.ceil(len/maxChars) — this test's + // job is to verify buildOverviewContent WIRES that packer's output into the right row shapes, + // not to re-derive the packing algorithm's own output. isOverview: true above -> 7-column + // shape -> USAGE_SAFE_TOKEN_CHARS_7COL is the right per-line floor input. + const expectedChunks = packUsageCellRowsWithMinimum( + [{ text: usageText }], + MAX_SAFE_USAGE_CHUNK_CHARS, + Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, USAGE_SAFE_TOKEN_CHARS_7COL), + ).length; expect(expectedChunks).toBeGreaterThan(1); // header (1) + expectedChunks rows for inv-long + 1 row for inv-next + 1 summary row. @@ -1281,16 +1492,20 @@ describe('buildOverviewContent — Usage chunking into continuation rows (scenar expect.any(String), ]); // Every subsequent (continuation) row: leading + amount cells are all blank, Usage cell is - // non-empty text. + // non-empty text (marker-stripped — #1940 AC5 prepends '… ' to every continuation row, see + // stripContinuationMarker). for (const contRow of longRows.slice(1)) { - const texts = rowTexts(contRow); + const texts = rowTexts(contRow, { isContinuation: true }); expect(texts.slice(0, 6)).toEqual(['', '', '', '', '', '']); expect(texts[6]).toBeTruthy(); } // I1: concatenating every chunk row's Usage text, in table order, reproduces the ORIGINAL - // usageText exactly — no character (including inter-chunk whitespace) is dropped. - const reconstructed = longRows.map((r) => rowTexts(r)[6]).join(''); + // usageText exactly — no character (including inter-chunk whitespace) is dropped. Row 0 is + // never a continuation row (isContinuation: i > 0), so its marker-free text stays as-is. + const reconstructed = longRows + .map((r, i) => rowTexts(r, { isContinuation: i > 0 })[6]) + .join(''); expect(reconstructed).toBe(usageText); // The next invoice's own (unrelated) row must still be present and unaffected, immediately @@ -1312,11 +1527,14 @@ describe('buildOverviewContent — Usage chunking into continuation rows (scenar const result = buildOverviewContent(content, new Map()); const table = getTable(result); - // Row count is driven by `packUsageCellRows` over the WHOLE cell stream (prose + suffix), not - // by the prose alone — see that function's own unit tests for the packing rules themselves. - const expectedRows = packUsageCellRows( + // Row count is driven by `packUsageCellRowsWithMinimum` over the WHOLE cell stream (prose + + // suffix) — the same formula buildOverviewContent itself calls (#1940 AC1), not the bare + // packUsageCellRows a runt-merge fix can change the row count of. isOverview defaults false + // above -> 6-column (claim) shape -> USAGE_SAFE_TOKEN_CHARS_6COL is the right per-line floor. + const expectedRows = packUsageCellRowsWithMinimum( [{ text: usageText }, { text: '\nGround Floor · 1 attachment: Invoice', meta: true }], MAX_SAFE_USAGE_CHUNK_CHARS, + Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, USAGE_SAFE_TOKEN_CHARS_6COL), ).length; expect(expectedRows).toBeGreaterThan(1); // header (1) + packed rows + summary row (1). @@ -1327,35 +1545,44 @@ describe('buildOverviewContent — Usage chunking into continuation rows (scenar // PLACEMENT (changed deliberately in the fix round): the suffix trails the prose, so it sits on // the LAST row — not row 0. Pinning it to row 0 rendered grey meta text mid-prose with more // usage text below it, and forced the prose's own chunk boundary to shrink to make room. + // usageRows.length === expectedRows > 1, so the last row is always index >= 1 (a continuation + // row) — strip its #1940 AC5 marker before inspecting. const lastRow = usageRows[usageRows.length - 1]!; const lastCell = lastRow[lastRow.length - 1] as { text: unknown; stack?: unknown }; expect(lastCell.stack).toBeUndefined(); - const last = splitUsageCell(lastCell); + const last = splitUsageCell(lastCell, { isContinuation: true }); expect(last.metaRun).not.toBeNull(); expect(last.metaRun!.text).toBe('\nGround Floor · 1 attachment: Invoice'); expect(last.metaRun!.color).toBe(GREY); - // Every EARLIER row is pure prose — the suffix is neither duplicated nor emitted early. - for (const earlierRow of usageRows.slice(0, -1)) { + // Every EARLIER row is pure prose — the suffix is neither duplicated nor emitted early. Row 0 + // (index 0 of usageRows) is never a continuation row; any further "earlier" row (index >= 1, + // possible when expectedRows > 2) is. + usageRows.slice(0, -1).forEach((earlierRow, i) => { const cell = earlierRow[earlierRow.length - 1] as { text: unknown; stack?: unknown }; expect(cell.stack).toBeUndefined(); - expect(splitUsageCell(cell).metaRun).toBeNull(); - } + expect(splitUsageCell(cell, { isContinuation: i > 0 }).metaRun).toBeNull(); + }); // I1 (no character is ever lost): concatenating only the usageText PORTION of every row's cell - // — i.e. excluding the grey meta suffix — reproduces the original usageText exactly. This is - // what proves the inline suffix didn't displace or truncate any prose. - const reconstructed = usageRows.map((r) => splitUsageCell(r[r.length - 1]).usageText).join(''); + // — i.e. excluding the grey meta suffix AND the #1940 marker — reproduces the original + // usageText exactly. This is what proves the inline suffix didn't displace or truncate any + // prose, and that the marker (render-time decoration only) never enters the reconstruction. + const reconstructed = usageRows + .map((r, i) => splitUsageCell(r[r.length - 1], { isContinuation: i > 0 }).usageText) + .join(''); expect(reconstructed).toBe(usageText); // Every row's Usage cell stays within the ONE page-safe budget — the bound that makes // `dontBreakRows: true` safe. Asserted against the constant, not the packer's own output. - for (const usageRow of usageRows) { - const { usageText: prose, metaRun } = splitUsageCell(usageRow[usageRow.length - 1]); + usageRows.forEach((usageRow, i) => { + const { usageText: prose, metaRun } = splitUsageCell(usageRow[usageRow.length - 1], { + isContinuation: i > 0, + }); expect(prose.length + (metaRun?.text.length ?? 0)).toBeLessThanOrEqual( MAX_SAFE_USAGE_CHUNK_CHARS, ); - } + }); // areaText/attachmentsNote text appears EXACTLY once across the WHOLE table — no duplication // onto other rows, no leakage into the summary row. @@ -1382,12 +1609,25 @@ describe('buildOverviewContent — Usage chunking into continuation rows (scenar // header (1) + prose row + suffix-only row + summary (1). expect(table.body).toHaveLength(4); + // Row 0 (the prose row) is never a continuation row — no #1940 marker. const proseRow = splitUsageCell((table.body[1] as unknown[])[5]); expect(proseRow.usageText).toBe(usageText); expect(proseRow.metaRun).toBeNull(); - const suffixRow = splitUsageCell((table.body[2] as unknown[])[5]); - // Body portion is the empty run buildUsageTextRuns emits for an absent prose segment. + // Row 1 (the suffix-only row) IS a continuation row. #1940 trace: this exact 645+37 fixture + // does NOT trigger AC1's runt-merge repack — packUsageCellRowsWithMinimum(6-col shape, + // minTrailingChars=22) packs the 37-char suffix into its own row at full budget already (37 >= + // 22, no runt), so packUsageCellRowsWithMinimum returns byte-identical row boundaries to plain + // packUsageCellRows here. The marker is still unconditionally prepended by buildUsageCell for + // every row index >= 1 regardless of whether AC1's merge fired — assert that directly first. + const suffixCellRaw = (table.body[2] as unknown[])[5] as { + text: { text: string; color?: string }[]; + }; + expect(suffixCellRaw.text[0]).toEqual({ text: '… ' }); + + const suffixRow = splitUsageCell((table.body[2] as unknown[])[5], { isContinuation: true }); + // Body portion is the empty run buildUsageTextRuns emits for an absent prose segment (after + // the marker itself is stripped). expect(suffixRow.usageText).toBe(''); expect(suffixRow.metaRun).not.toBeNull(); // NO leading newline — this is the whole point of the assertion. @@ -1443,6 +1683,152 @@ describe('buildOverviewContent — Usage chunking into continuation rows (scenar }); }); +describe('AC5 (#1940): the "… " continuation marker — placement and exact run shape', () => { + it('(a) multiple continuation rows, no meta suffix: row 0 never starts with the marker; every row i>=1 starts with EXACTLY { text: "… " } — no color, bold, or fontSize', () => { + const usageText = proseOfLength(MAX_SAFE_USAGE_CHUNK_CHARS * 5); + const row = makeRow({ invoiceId: 'inv-1', vendor: 'Marker Vendor', usageText }); + const content = makeContent({ rows: [row] }); // isOverview default false -> 6-column shape + const table = getTable(buildOverviewContent(content, new Map())); + + const expectedRows = packUsageCellRowsWithMinimum( + [{ text: usageText }], + MAX_SAFE_USAGE_CHUNK_CHARS, + Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, USAGE_SAFE_TOKEN_CHARS_6COL), + ).length; + expect(expectedRows).toBeGreaterThan(1); + + const usageRows = table.body.slice(1, 1 + expectedRows) as { text: unknown }[][]; + + const row0Cell = usageRows[0]![usageRows[0]!.length - 1] as { + text: { text: string; color?: string }[]; + }; + expect(row0Cell.text[0]!.text).not.toBe('… '); + + for (const contRow of usageRows.slice(1)) { + const cell = contRow[contRow.length - 1] as { + text: { text: string; color?: string; bold?: boolean; fontSize?: number }[]; + }; + // Exact run shape — not just "starts with the right text" but no stray styling either. + expect(cell.text[0]).toEqual({ text: '… ' }); + } + }); + + it('(b) the meta-suffix-on-its-own-row case: the marker still comes first, then the grey run whose text no longer starts with "\\n"', () => { + const usageText = proseOfLength(645); + const row = makeRow({ + invoiceId: 'inv-1', + usageText, + areaText: 'Ground Floor', + attachmentsNote: '1 attachment: Invoice', + }); + const table = getTable(buildOverviewContent(makeContent({ rows: [row] }), new Map())); + + const suffixCell = (table.body[2] as unknown[])[5] as { + text: { text: string; color?: string }[]; + }; + expect(suffixCell.text[0]).toEqual({ text: '… ' }); + expect(suffixCell.text[1]!.color).toBe(GREY); + expect(suffixCell.text[1]!.text.startsWith('\n')).toBe(false); + }); +}); + +describe('AC8 (#1940): content at/below the zero-degradation range never produces a continuation row', () => { + it('MAX_SAFE_USAGE_CHUNK_CHARS characters still yields exactly ONE row for the 7-column shape — unchanged by the runt-merge fix', () => { + const usageText = proseOfLength(MAX_SAFE_USAGE_CHUNK_CHARS); + const row = makeRow({ usageText }); + const content = makeContent({ isOverview: true, rows: [row] }); + const table = getTable(buildOverviewContent(content, new Map())); + // header (1) + exactly 1 row + 1 summary row. + expect(table.body).toHaveLength(3); + }); + + it('600 characters yields exactly ONE row at the widest legal subset ({allocatedAmount, usage}) — derived via computeColumnWidths, not hand-computed', () => { + const { widths } = computeColumnWidths(['allocatedAmount', 'usage']); + // Sanity: this subset's Usage width really is wider than the 7-column reference (a real, + // legal #1973 subset, not an invented one) — a failure here means the fixture premise + // ("widest legal subset") silently stopped holding, not that AC8 broke. + expect(widths.usage!).toBeGreaterThan(USAGE_WIDTH_7COL); + const usageChunkChars = usageChunkCharsForWidth(widths.usage!); + // usageChunkCharsForWidth's one-sided clamp (AC 3.7) means a WIDER subset never exceeds + // MAX_SAFE_USAGE_CHUNK_CHARS — confirmed directly rather than assumed >= 600. + expect(usageChunkChars).toBe(MAX_SAFE_USAGE_CHUNK_CHARS); + + const usageText = proseOfLength(600); + const row = makeRow({ usageText }); + const hiddenColumns = new Set([ + 'vendor', + 'invoiceNumber', + 'date', + 'status', + 'invoiceAmount', + ]); + const content = makeContent({ isOverview: true, rows: [row] }); + const table = getTable(buildOverviewContent(content, new Map(), hiddenColumns)); + + // Confirm the subset genuinely rendered as {allocatedAmount, usage} — not silently something + // wider — so this test can't pass vacuously if the hidden-set derivation above were wrong. + expect(table.widths).toHaveLength(2); + // header (1) + exactly 1 row. No leading column and no invoiceAmount column survive at this + // subset, so summary rows render as a separate stack block below the table (Tier 3, R2) rather + // than a table row — table.body has no third row to account for here. + expect(table.body).toHaveLength(2); + }); +}); + +describe('AC2 (#1940): every emitted row still fits the page-safe ceiling after the runt merge', () => { + it('a token engineered to hard-split into a runt at the 7-column shape: every row (marker excluded) stays within usageChunkChars, and no continuation row drops below minTrailingUsageChars', () => { + const { widths } = computeColumnWidths(reportColumnsForUseCase(true) as ReportColumnKey[]); + const usageChunkChars = usageChunkCharsForWidth(widths.usage!); + // Two deliberately distinct jobs, not a redundant pair: usageSafeTokenCharsForWidth does the + // main work of guaranteeing a merged runt fills one real line at THIS subset's width; + // MIN_CONTINUATION_ROW_FLOOR_CHARS is the absolute fallback floor for subsets where that + // per-line figure is itself small. Do not collapse this to either operand alone. + const minTrailingUsageChars = Math.max( + MIN_CONTINUATION_ROW_FLOOR_CHARS, + usageSafeTokenCharsForWidth(widths.usage!), + ); + + // Today's exact pathology: a whitespace-free run of 2*usageChunkChars + 1 hard-splits to + // [usageChunkChars, usageChunkChars, 1] under plain packing — a 1-character runt row. + const token = 'z'.repeat(usageChunkChars * 2 + 1); + const row = makeRow({ usageText: token }); + const content = makeContent({ isOverview: true, rows: [row] }); + const table = getTable(buildOverviewContent(content, new Map())); + + const dataRows = table.body.slice(1, table.body.length - 1) as unknown[][]; + expect(dataRows.length).toBeGreaterThan(1); + + let reconstructed = ''; + dataRows.forEach((r, i) => { + const { usageText } = splitUsageCell(r[r.length - 1], { isContinuation: i > 0 }); + reconstructed += usageText; + expect(usageText.length).toBeLessThanOrEqual(usageChunkChars); + if (i > 0) { + expect(usageText.length).toBeGreaterThanOrEqual(minTrailingUsageChars); + } + }); + expect(reconstructed).toBe(token); + }); + + it('AC4 regression: a whitespace-free run of usageChunkChars * 3 still terminates, every row <= usageChunkChars, and reconstructs exactly', () => { + const { widths } = computeColumnWidths(reportColumnsForUseCase(true) as ReportColumnKey[]); + const usageChunkChars = usageChunkCharsForWidth(widths.usage!); + const token = 'q'.repeat(usageChunkChars * 3); + const row = makeRow({ usageText: token }); + const content = makeContent({ isOverview: true, rows: [row] }); + const table = getTable(buildOverviewContent(content, new Map())); + + const dataRows = table.body.slice(1, table.body.length - 1) as unknown[][]; + let reconstructed = ''; + dataRows.forEach((r, i) => { + const { usageText } = splitUsageCell(r[r.length - 1], { isContinuation: i > 0 }); + reconstructed += usageText; + expect(usageText.length).toBeLessThanOrEqual(usageChunkChars); + }); + expect(reconstructed).toBe(token); + }); +}); + describe('buildOverviewContent — AC14: falsy statusText never produces a malformed row (scenario 12)', () => { it('an overview row with statusText: "" still produces a 7-cell row with an empty-text status cell, and does not throw', () => { const row = makeRow({ statusText: '' }); diff --git a/client/src/lib/reportPdf/overviewPdf.ts b/client/src/lib/reportPdf/overviewPdf.ts index 2ed536299..4778e5eab 100644 --- a/client/src/lib/reportPdf/overviewPdf.ts +++ b/client/src/lib/reportPdf/overviewPdf.ts @@ -394,6 +394,71 @@ export function packUsageCellRows( return rows.length > 0 ? rows : [[{ text: '' }]]; } +/** + * Row-total character count for a packed Usage-cell row (sum of every segment's text length). + */ +function rowCharCount(row: UsageCellSegment[]): number { + return row.reduce((sum, segment) => sum + segment.text.length, 0); +} + +/** + * Wraps `packUsageCellRows` with a floor on every continuation row's (row index >= 1) character + * count, so a would-be runt remainder never renders as its own near-empty row (#1940 AC1). + * + * Why the runt check gates the repack rather than always packing at a reduced budget: packing + * once at the FULL `maxChars` budget and returning that result untouched whenever no row is a + * runt preserves today's exact row counts for everything that already fits one row (AC8) and for + * multi-row content that already divides cleanly — both cases are real-render-verified elsewhere + * in this file (see MAX_SAFE_USAGE_CHUNK_CHARS). Unconditionally packing at `maxChars - + * minTrailingChars` would split some of that content into an extra row for no reason, regressing + * the "content at the ceiling renders as one row" behaviour AC8 exists to pin. + * + * AC2 safety argument (every row this returns fits the page-safe budget it was originally called + * with): `packUsageCellRows` guarantees every row it returns is <= the `maxChars` it was called + * with. In the repack path here, every raw row from `packUsageCellRows(segments, maxChars - + * minTrailingChars)` is therefore <= `maxChars - minTrailingChars`. The backward scan below merges + * `rows[i]` into `rows[i - 1]` only when `rowCharCount(rows[i]) < minTrailingChars`, and because + * the scan runs backward, `rows[i - 1]` (the receiver) has never yet been grown by an earlier + * merge in this same pass when it receives one — so every merge produces a row of size + * `(<= maxChars - minTrailingChars) + (< minTrailingChars) < maxChars`, and this bound holds at + * any cascade depth (a receiver can itself be merged into its predecessor later in the same + * backward pass, but by the same argument applied again). + * + * Termination: a single backward pass over an array that only shrinks (`splice` removes, never + * inserts) — `packUsageCellRows` is invoked exactly once up front and never again inside the loop, + * so there is no risk of the repack recursing or re-deriving a smaller and smaller budget. + * + * The `minTrailingChars <= 0 || minTrailingChars >= maxChars` guard degrades to the plain + * `packUsageCellRows` output rather than throwing: unlike `packUsageCellRows`/ + * `splitIntoPageSafeChunks` (which throw on a non-positive `maxChars` because that budget can + * never be survived), a degenerate `minTrailingChars` here just means "no runt-merge floor" is + * applicable, and the real caller always derives it as a positive value strictly below the chunk + * ceiling — so falling back to unmerged rows is the correct behaviour for an edge case that isn't + * expected to occur, rather than crashing a report render over a decoration threshold. + */ +export function packUsageCellRowsWithMinimum( + segments: UsageCellSegment[], + maxChars: number, + minTrailingChars: number, +): UsageCellSegment[][] { + if (minTrailingChars <= 0 || minTrailingChars >= maxChars) { + return packUsageCellRows(segments, maxChars); + } + const rawRows = packUsageCellRows(segments, maxChars); + const hasRunt = rawRows.slice(1).some((row) => rowCharCount(row) < minTrailingChars); + if (!hasRunt) { + return rawRows; + } + const rows = packUsageCellRows(segments, maxChars - minTrailingChars); + for (let i = rows.length - 1; i >= 1; i--) { + if (rowCharCount(rows[i]!) < minTrailingChars) { + rows[i - 1] = rows[i - 1]!.concat(rows[i]!); + rows.splice(i, 1); + } + } + return rows; +} + /** * A whitespace-free run at or under this many characters is guaranteed to fit on one line within * the given column width, EVEN IN THE WORST CASE (every character as wide as 'W'), and renders @@ -608,6 +673,26 @@ export const HEADER_ROW_HEIGHT_MAX = * - `markerText` (see above) is the one remaining unbounded row-height contributor in this table. */ +/** + * Absolute floor (characters) for a continuation row's runt-merge threshold (#1940 AC1), used as + * the lower bound of `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, usageSafeTokenChars)` below. + * + * The runt-merge threshold has two different jobs, and this constant is only the second one: + * `usageSafeTokenChars` (the per-subset, per-line character budget already computed for word- + * break protection) does the main work — it guarantees a merged runt fills at least one real line + * at the CURRENT subset's actual width, so "does this look like a real line of content" holds + * regardless of which of the 96 legal subsets (#1973) produced it. This constant is the fallback + * for the case where that per-line figure is itself very small (a narrow Usage column at a small + * font): 20 characters reads as "clearly more than a stray word or character" even in isolation, + * independent of any subset's width, so the merge threshold never degrades below a value that + * still looks like real content on its own. + * + * Named and exported (not inlined) per #1950: a numeric threshold that appears in test assertions + * needs one source of truth, so a future ux-designer tuning of the floor changes this one constant + * rather than silently drifting between production and every test call site that re-derives it. + */ +export const MIN_CONTINUATION_ROW_FLOOR_CHARS = 20; + export function buildOverviewContent( reportContent: ReportContent, skippedDocuments: Map, @@ -789,6 +874,15 @@ export function buildOverviewContent( const usageSafeTokenChars = usageVisible ? usageSafeTokenCharsForWidth(colWidths.usage!) : 0; const usageChunkChars = usageVisible ? usageChunkCharsForWidth(colWidths.usage!) : 0; + // AC1 (#1940): floor for a continuation row so a would-be runt remainder merges into the row + // before it instead of rendering as a near-empty row indistinguishable from a broken document. + // Expressed via this subset's own per-line character budget (ux-designer recommendation), with + // MIN_CONTINUATION_ROW_FLOOR_CHARS as the absolute lower bound — see that constant's doc comment + // for why the threshold needs both terms. + const minTrailingUsageChars = usageVisible + ? Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, usageSafeTokenChars) + : 0; + /** * Renders one packed row's worth of Usage-cell segments (see packUsageCellRows) into a cell. * @@ -796,9 +890,22 @@ export function buildOverviewContent( * protection. Each meta run is coloured DEPOSIT_NOTE_TEXT_COLOR after the split, so a cell may * hold multiple consecutive grey runs — they are always last (relied on by splitUsageCell in * tests and by any caller reading these cells back). + * + * `isContinuation` (#1940 AC5) prepends a single literal `'… '` run (U+2026 + space) as the very + * first run when true, per the ux-designer's visual spec (issue #1940 comment). This is pure + * render-time decoration: it is never counted against `packUsageCellRowsWithMinimum`'s character + * budget and never part of the #1929 I1 reconstruction, since neither `packUsageCellRows`/ + * `packUsageCellRowsWithMinimum` nor `UsageCellSegment.text` ever see this string — it is added + * here, after packing, purely for what gets rendered. Deliberately NO colour override (not + * `DEPOSIT_NOTE_TEXT_COLOR`): the ux-designer chose an ink-shape signal over a colour/fill signal + * precisely because colour is what degrades under greyscale printing and photocopying, and this + * is a bank document that gets scanned — do not "helpfully" add a colour here. */ - function buildUsageCell(segments: UsageCellSegment[]): Content { + function buildUsageCell(segments: UsageCellSegment[], isContinuation = false): Content { const runs: Content[] = []; + if (isContinuation) { + runs.push({ text: '… ' }); + } segments.forEach((segment, index) => { if (!segment.meta) { runs.push(...buildUsageTextRuns(segment.text, usageSafeTokenChars)); @@ -879,17 +986,26 @@ export function buildOverviewContent( // overflow instead of paginating it (#1929 architect review HIGH 4 / I1). Wherever the whole // cell fits one row — the common case — this emits exactly one row, unchanged from #1959. // The first packed row shares this invoice's leading/amount cells; any further row is a - // Usage-only continuation row with no "continued" marker, per the product-owner's ruling - // (#1929 AC2/AC4/AC12). + // Usage-only continuation row (#1929 AC2/AC4/AC12), now carrying the leading '… ' marker + // buildUsageCell adds for isContinuation rows (#1940 AC5) — the earlier "no marker" ruling + // was superseded once round-3/4 renders showed a markerless continuation row is visually + // indistinguishable from a broken/orphaned one. const cellSegments: UsageCellSegment[] = [{ text: contentRow.usageText }]; if (metaPieces.length > 0) { cellSegments.push({ text: `\n${metaPieces.join(' · ')}`, meta: true }); } - const packedCellRows = packUsageCellRows(cellSegments, usageChunkChars); + const packedCellRows = packUsageCellRowsWithMinimum( + cellSegments, + usageChunkChars, + minTrailingUsageChars, + ); rows.push([...nonUsageCells, buildUsageCell(packedCellRows[0]!)]); for (let i = 1; i < packedCellRows.length; i++) { - rows.push([...nonUsageVisible.map(buildEmptyBodyCell), buildUsageCell(packedCellRows[i]!)]); + rows.push([ + ...nonUsageVisible.map(buildEmptyBodyCell), + buildUsageCell(packedCellRows[i]!, true), + ]); } } diff --git a/client/src/lib/reportPdf/realRender.test.ts b/client/src/lib/reportPdf/realRender.test.ts index 795d42ab4..1b1180c5d 100644 --- a/client/src/lib/reportPdf/realRender.test.ts +++ b/client/src/lib/reportPdf/realRender.test.ts @@ -80,6 +80,9 @@ import { USAGE_WIDTH_6COL, MAX_SAFE_USAGE_CHUNK_CHARS, HEADER_ROW_HEIGHT_MAX, + USAGE_SAFE_TOKEN_CHARS_7COL, + USAGE_SAFE_TOKEN_CHARS_6COL, + MIN_CONTINUATION_ROW_FLOOR_CHARS, } from './overviewPdf.js'; import type { UsageCellSegment } from './overviewPdf.js'; @@ -347,18 +350,41 @@ function usageCellText(text: unknown): string { // page-count tripwire fail for the wrong reason instead of flipping. const META_GREY = '#6b7280'; -function splitUsageCell(cell: unknown): { +// #1940 AC5: buildUsageCell prepends a single literal `{ text: '… ' }` run (U+2026 + space, no +// `color` override) as the very first run of a CONTINUATION row's (packedCellRows index >= 1) +// Usage cell — the visual "this row continues" signal. `splitUsageCell` below concatenates a +// cell's ENTIRE run array into `usageText`, so without stripping, every continuation-row +// reconstruction in this file would silently start comparing `'… ' + text` against `text`. +// Centralized HERE, once: strips exactly one leading run whose `.text === '… '` and which carries +// no `color`. Only ever applied when the CALLER passes `isContinuation: true` — never applied +// unconditionally, which would risk masking a genuine reconstruction defect that happened to +// produce a leading '… ' in real user content on a row that was never supposed to carry it. +function stripContinuationMarker(runs: T[]): T[] { + if (runs.length > 0 && runs[0]!.text === '… ' && runs[0]!.color === undefined) { + return runs.slice(1); + } + return runs; +} + +function splitUsageCell( + cell: unknown, + opts: { isContinuation?: boolean } = {}, +): { usageText: string; /** Grey suffix exactly as rendered, INCLUDING any leading '\n' — for faithful concatenation. */ metaRaw: string | null; /** Grey suffix with the presentational leading '\n' stripped — for single-cell assertions. */ metaText: string | null; } { - const runs = (cell as { text: unknown }).text; - if (!Array.isArray(runs)) { - return { usageText: runs as string, metaRaw: null, metaText: null }; + const rawRuns = (cell as { text: unknown }).text; + if (!Array.isArray(rawRuns)) { + return { usageText: rawRuns as string, metaRaw: null, metaText: null }; } - const typed = runs as { text: string; color?: string }[]; + const typed = ( + opts.isContinuation + ? stripContinuationMarker(rawRuns as { text: string; color?: string }[]) + : rawRuns + ) as { text: string; color?: string }[]; const greyIndexes = typed .map((run, i) => (run.color === META_GREY ? i : -1)) .filter((i) => i !== -1); @@ -2291,9 +2317,11 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { expect(chunkRows.length).toBeGreaterThan(1); // multiple rows, per AC12's "well over" side // #1929 round 2: each row's Usage cell `.text` is now a run array (buildUsageTextRuns), not - // a plain string — reconstruct each row's runs before concatenating across rows. + // a plain string — reconstruct each row's runs before concatenating across rows. #1940 AC5: + // every row but the first (index 0) in this group is a continuation row and carries the '… ' + // marker — strip it via splitUsageCell (usageCellText alone doesn't know about the marker). const reconstructed = chunkRows - .map((row) => usageCellText((row[row.length - 1] as { text?: unknown })?.text ?? '')) + .map((row, i) => splitUsageCell(row[row.length - 1], { isContinuation: i > 0 }).usageText) .join(''); expect(reconstructed).toBe(usageText); }); @@ -2445,7 +2473,8 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { /** Real page count of the rendered PDF. */ pageCount: number; }> { - const { buildOverviewContent, packUsageCellRows } = await import('./overviewPdf.js'); + const { buildOverviewContent, packUsageCellRowsWithMinimum } = + await import('./overviewPdf.js'); const content = makeCellScopeContent(rowOverrides); const row = content.rows[0]!; const pdfContent = buildOverviewContent(content, new Map()); @@ -2456,9 +2485,17 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { ); const tableItem = findTableItem(pdfContent); - const packedRowCount = packUsageCellRows( + // #1940: production now packs via packUsageCellRowsWithMinimum (AC1's runt-remainder merge), + // not the bare packUsageCellRows — the row count this helper expects must be derived from + // the SAME formula buildOverviewContent itself calls, or a genuine runt-merge in one of this + // block's large fixtures would make this length assertion (and the "row was SILENTLY + // DROPPED" guard below) fail for an unrelated reason. makeCellScopeContent's content has + // isOverview: false -> 6-column (claim) shape -> USAGE_SAFE_TOKEN_CHARS_6COL is the right + // per-line floor input. + const packedRowCount = packUsageCellRowsWithMinimum( cellSegmentsFor(row), MAX_SAFE_USAGE_CHUNK_CHARS, + Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, USAGE_SAFE_TOKEN_CHARS_6COL), ).length; // header (1) + packedRowCount + summary (1) — if this length check fails, a row was @@ -2467,7 +2504,12 @@ describe('report PDF pipeline — real, unmocked end-to-end render', () => { expect(tableItem.table.body).toHaveLength(1 + packedRowCount + 1); const dataRows = tableItem.table.body.slice(1, 1 + packedRowCount) as unknown[][]; - const parts = dataRows.map((r) => splitUsageCell(r[r.length - 1])); + // #1940 AC5: every row but the first (index 0) is a continuation row and carries the '… ' + // marker buildUsageCell prepends — strip it before reconstructing usageText/metaRaw, or the + // marker leaks into usageText (it is never grey, so it always lands in the non-grey prefix). + const parts = dataRows.map((r, i) => + splitUsageCell(r[r.length - 1], { isContinuation: i > 0 }), + ); const metaRaw = parts.map((p) => p.metaRaw ?? '').join(''); const pdfDoc = await PDFDocument.load(await blob.arrayBuffer()); return { @@ -3201,6 +3243,201 @@ describe('ADR-034 rule #1: horizontal-overflow via _minWidth <= _calcWidth (issu ); }); +// ─── #1940: continuation-row marker (AC5) and runt-avoidance (AC1/AC9), real render ──────────── +// +// AC9 explicitly asks for a real-render pin that no near-empty continuation row is emitted for +// text engineered to land just past a chunk boundary, plus verification (not assumption) that the +// marker glyph U+2026 actually paints in the embedded font. AC7 asks for the same +// _minWidth <= _calcWidth / same-page-number proof this file already applies elsewhere (see the +// ADR-034 block directly above), at the per-subset ceiling. +describe('#1940: continuation-row marker (AC5) and runt-avoidance (AC1/AC9), real render', () => { + // Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, USAGE_SAFE_TOKEN_CHARS_7COL) — 16 < 20 at the + // 7-column shape, so the floor is MIN_CONTINUATION_ROW_FLOOR_CHARS (20) itself here. The two + // operands are deliberately distinct: the per-line figure does the main work at wider subsets, + // the named floor is the fallback for narrow ones — see that constant's own doc comment. + const MIN_TRAILING_7COL = Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, USAGE_SAFE_TOKEN_CHARS_7COL); + + // Minimal hand-built ReportContent, mirroring the "cell-scope invariant" block's own + // makeCellScopeContent fixture above (same file convention: scope fixture helpers locally per + // describe block). isOverview: true -> 7-column shape, matching MAX_SAFE_USAGE_CHUNK_CHARS's own + // measured reference geometry (see that constant's doc comment in overviewPdf.ts). + function makeMarkerContent(usageText: string): ReportContent { + const row: ReportContentRow = { + invoiceId: 'inv-marker', + vendor: 'Marker Vendor', + invoiceNumber: 'MRK-1', + dateText: '01/01/2026', + status: null, + statusText: 'Pending', + invoiceAmountText: '€100.00', + allocatedAmountValueText: '€100.00', + isSplit: false, + isDepositReduced: false, + isDeposit: false, + isRefund: false, + refundNoteText: '', + usageText, + attachmentsNote: null, + areaText: null, + }; + return { + isOverview: true, + isClaim: false, + tableTitle: 'Marker Test Report', + labels: { + vendor: 'Vendor', + invoiceNumber: 'Invoice No.', + date: 'Date', + status: 'Status', + invoiceAmount: 'Invoice Amount', + allocatedAmount: 'Allocated Amount', + usage: 'Usage', + attachmentsNote: 'Attachments', + deposit: 'Deposit', + splitNote: 'partial', + depositReducedNote: 'less deposit', + source: 'Source', + sourceType: 'Source Type', + reference: 'Reference', + generatedAt: 'Generated At', + pageLabel: 'Page', + coverLetterReferenceLabel: 'Reference:', + coverLetterSubjectLabel: 'Subject:', + skipReasonLabels: { + footnoteFetchFailed: 'Fetch failed', + footnoteInvalidPdf: 'Invalid PDF', + }, + }, + sourceInfo: { + sourceName: 'Marker Source', + sourceTypeText: 'Bank Loan', + referenceText: null, + generatedAtText: '01/01/2026', + }, + coverLetter: null, + rows: [row], + summaryRows: [{ key: 'total', label: 'Total', amountText: '€100.00' }], + footnotes: [], + }; + } + + // Same pathological construction as the unit-level AC1/AC2 tests in overviewPdf.test.ts: a + // whitespace-free run of 2*MAX_SAFE_USAGE_CHUNK_CHARS + 1 hard-splits to [chunk, chunk, 1] under + // PLAIN packing — a single-character last row, the exact "one letter" reproduction #1940 was + // filed from (issue body, round-4 ux-designer finding). + async function renderRuntBoundaryFixture(): Promise<{ + dataRows: unknown[][]; + tableItem: { table: RenderedTable }; + }> { + const { buildOverviewContent } = await import('./overviewPdf.js'); + const token = 'z'.repeat(MAX_SAFE_USAGE_CHUNK_CHARS * 2 + 1); + const content = makeMarkerContent(token); + const pdfContent = buildOverviewContent(content, new Map()); + await renderOverviewPdfContent( + pdfContent, + { tableTitle: content.tableTitle, sourceName: content.sourceInfo.sourceName }, + tEn, + ); + const tableItem = findTableItem(pdfContent); + const dataRows = tableItem.table.body.slice(1, tableItem.table.body.length - 1) as unknown[][]; + return { dataRows, tableItem }; + } + + it("[AC9] a runt-boundary fixture renders with NO near-empty continuation row: the last row's marker-stripped Usage content is substantial, and the full token is recoverable across the group", async () => { + const token = 'z'.repeat(MAX_SAFE_USAGE_CHUNK_CHARS * 2 + 1); + const { dataRows } = await renderRuntBoundaryFixture(); + expect(dataRows.length).toBeGreaterThan(1); + + const lastRow = dataRows[dataRows.length - 1]!; + const { usageText } = splitUsageCell(lastRow[lastRow.length - 1], { isContinuation: true }); + // The pre-fix pathology this AC exists to close: a near-empty (1-character) last row. The + // fix's own floor guarantees at least MIN_TRAILING_7COL characters here — this is the direct + // negation of "a table row that was entirely blank except for a single stray character". + expect(usageText.length).toBeGreaterThanOrEqual(MIN_TRAILING_7COL); + + // Full losslessness across the group (I1), marker excluded from the reconstruction. + const reconstructed = dataRows + .map((r, i) => splitUsageCell(r[r.length - 1], { isContinuation: i > 0 }).usageText) + .join(''); + expect(reconstructed).toBe(token); + }); + + it('[AC5] every continuation row starts with the literal "… " marker in the real rendered content tree; row 0 never does', async () => { + const { dataRows } = await renderRuntBoundaryFixture(); + expect(dataRows.length).toBeGreaterThan(1); + + const row0Cell = dataRows[0]![dataRows[0]!.length - 1] as { + text: { text: string; color?: string }[]; + }; + expect(row0Cell.text[0]!.text).not.toBe('… '); + + for (const contRow of dataRows.slice(1)) { + const cell = contRow[contRow.length - 1] as { text: { text: string; color?: string }[] }; + expect(cell.text[0]).toEqual({ text: '… ' }); + } + }); + + it('[AC9, glyph coverage] the marker glyph U+2026 (HORIZONTAL ELLIPSIS) has a real, non-.notdef glyph in the embedded Roboto-Regular font', async () => { + // Verified rather than assumed by U+2014 (em dash) precedent elsewhere in this file — same + // General Punctuation Unicode block, but a different codepoint, and per the ux-designer's own + // review comment, "this file's whole culture is measured, not estimated". fontkit is a + // transitive dependency of pdfmake (via pdfkit) already present in node_modules; pinned here as + // an explicit devDependency (client/package.json) per the Dependency Policy rather than relying + // on an undeclared transitive resolution. + // + // Glyph id 0 is `.notdef` by spec — the exact "tofu box" failure mode this check rules out. A + // raw advance-width measurement would NOT prove this: fonts commonly give `.notdef` a real, + // non-zero advance width, so only the glyph id itself is dispositive. + const fontkitModule = await import('fontkit'); + const fontkit = + (fontkitModule as unknown as { default?: typeof fontkitModule }).default ?? fontkitModule; + const vfsModule = await import('pdfmake/build/vfs_fonts'); + const vfs = + (vfsModule as { default?: Record }).default ?? + (vfsModule as unknown as Record); + const base64 = vfs['Roboto-Regular.ttf']; + if (typeof base64 !== 'string') { + throw new Error( + 'Roboto-Regular.ttf not found in pdfmake vfs_fonts — loader.ts font wiring changed?', + ); + } + const font = fontkit.create(Buffer.from(base64, 'base64')); + const glyph = font.glyphForCodePoint(0x2026); + expect(glyph.id).not.toBe(0); + + // Sanity check on the technique itself: a genuinely unmapped astral codepoint DOES resolve to + // glyph id 0 in this font — proves glyph id 0 is reachable and meaningful here, not a fontkit + // quirk that always returns non-zero. + const unmapped = font.glyphForCodePoint(0x10ffff); + expect(unmapped.id).toBe(0); + }); + + it('[AC7] at the per-subset ceiling, the Usage column has no horizontal overflow (_minWidth <= _calcWidth) and every cell of the continuation row shares one real page number', async () => { + const { dataRows, tableItem } = await renderRuntBoundaryFixture(); + expect(dataRows.length).toBeGreaterThan(1); + + const usageColIndex = tableItem.table.widths.length - 1; + const usageCalcWidth = calcWidthsOf(tableItem.table.widths)[usageColIndex]!; + + const contRow = dataRows[dataRows.length - 1] as { _minWidth?: number }[]; + const usageCell = contRow[usageColIndex] as { _minWidth?: number }; + const minWidth = usageCell._minWidth; + if (typeof minWidth !== 'number') { + throw new Error('usageCell._minWidth is not a number — was the render awaited first?'); + } + expect(minWidth).toBeLessThanOrEqual(usageCalcWidth); + + // Every cell of this continuation row — INCLUDING the blank leading/amount cells + // buildEmptyBodyCell produces — lands on the SAME real page: `dontBreakRows: true` held for + // the whole row, not just the Usage cell. A blank-text cell still receives a real + // `.positions` entry from pdfmake's layout engine (confirmed empirically before writing this + // assertion: an empty-string cell gets exactly one position, the same shape a non-empty cell + // gets), so `cellPageNumber` is meaningful on every cell here, not just the Usage one. + const pageNumbers = contRow.map((cell) => cellPageNumber(cell)); + expect(new Set(pageNumbers).size).toBe(1); + }); +}); + // ─── #1980: legend sentence layout and occurrence count ─────────────────────────────────────────── // // Exercises the document-level legend entries (content.footnotes[]) added by issue #1965: diff --git a/package-lock.json b/package-lock.json index 07ae1c5e7..af5c87b9e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -60,6 +60,7 @@ "@babel/core": "7.29.7", "@babel/preset-react": "7.29.7", "@babel/preset-typescript": "7.29.7", + "@types/fontkit": "2.0.4", "@types/pdfmake": "0.3.3", "@types/react": "19.2.18", "@types/react-dom": "19.2.4", @@ -67,6 +68,7 @@ "copy-webpack-plugin": "14.0.0", "css-loader": "7.1.4", "css-minimizer-webpack-plugin": "8.0.0", + "fontkit": "2.0.4", "html-webpack-plugin": "5.6.8", "mini-css-extract-plugin": "2.10.2", "style-loader": "4.0.0", @@ -10160,6 +10162,16 @@ "@types/send": "*" } }, + "node_modules/@types/fontkit": { + "version": "2.0.4", + "resolved": "https://registry.npmjs.org/@types/fontkit/-/fontkit-2.0.4.tgz", + "integrity": "sha512-+D925Yyu7sVxqE091SGnMvvp+gD20obGj52wwTRnhqpj2M0FhT5mZLo7MW23pwS/hKzzu1Kux7VlbFIGTJVeNA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/hast": { "version": "3.0.5", "resolved": "https://registry.npmjs.org/@types/hast/-/hast-3.0.5.tgz", @@ -34910,16 +34922,6 @@ "node": ">=0.8.0" } }, - "node_modules/undici": { - "version": "8.10.0", - "resolved": "https://registry.npmjs.org/undici/-/undici-8.10.0.tgz", - "integrity": "sha512-HvltHd7avK13QIw/oLe4qoOLyoVSoafqJ2jYOrtMRBkbYT31eiBQ8O0ehRKZiEZCMEyLFQNIADpgCWC5fALvYQ==", - "dev": true, - "license": "MIT", - "engines": { - "node": ">=22.19.0" - } - }, "node_modules/undici-types": { "version": "8.3.0", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-8.3.0.tgz", From 7a8f2e73953e8dbc83f8ec3608e926b8d8723608 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 05:50:19 +0200 Subject: [PATCH 2/2] docs(memory): record the AC1/AC2 resolution and greyscale-robustness reasoning - Records why the runt-merge wraps packUsageCellRows' row-level output rather than changing the chunker: the AC's unit is the rendered row, while the chunker's unit is a chunk within one segment, and a row can hold a prose chunk plus the grey meta segment. The two-code-paths argument is true but the unit mismatch is the load-bearing reason. - Records the AC2 induction and that it was confirmed by fuzzing 400,000 cases rather than a third hand-trace: zero AC1/AC2/I1 violations, max overage 0. - Records that the marker is ink shape rather than colour deliberately, since colour is what degrades under the greyscale printing and photocopying a bank document undergoes, and that the visual outcome was confirmed by rasterizing real renders at the narrowest and widest legal subsets rather than inferred. - Records that the threshold-to-ceiling ratio does NOT stay constant across subsets -- the one-sided clamp pins the ceiling while the floor scales with width -- so that framing must not be reused as a justification. Refs #1940 Co-Authored-By: Claude product-architect Co-Authored-By: Claude ux-designer --- .../agent-memory/product-architect/MEMORY.md | 4 +- .../product-architect/client-pdf-pipeline.md | 56 +++++++++++++++++++ .../product-architect/recurring-patterns.md | 28 ++++++++++ .claude/agent-memory/ux-designer/MEMORY.md | 2 +- .../pdfmake-rendering-verification.md | 31 ++++++++++ 5 files changed, 118 insertions(+), 3 deletions(-) diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index 8174f0ef9..51155fde6 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, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\`` doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python `len()` not `awk length()` (em-dashes are 3 bytes) + two open findings: the `BACKUP_DIR` default/gate is wrong on Architecture.md and API-Contract.md's `splitKind` table is a latent `format:check` failure (#1992, wiki `e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms` while enforcing a regex subset, a caution box falsified by `parseInt` leniency, and `trustProxy: 1` being a hop count not "trust all proxies" (#1990, PR #2027)** +- [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, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\`` doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python `len()` not `awk length()` (em-dashes are 3 bytes) + two open findings: the `BACKUP_DIR` default/gate is wrong on Architecture.md and API-Contract.md's `splitKind` table is a latent `format:check` failure (#1992, wiki `e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms` while enforcing a regex subset, a caution box falsified by `parseInt` leniency, and `trustProxy: 1` being a hop count not "trust all proxies" (#1990, PR #2027)**, **fuzz the verbatim ports when a doc comment carries an induction proof (a hand-trace only re-reads the author's argument) + a safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940, PR #2032)** - [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 array-shape gates are proxies; **`splitKind` SHIPPED #1911/PR #2015** incl. the ≠S-per-arm predicate, the residual arithmetic proving `(less deposit)` in both directions, the UNION-dedup/`COUNT(*)` trap, and why `isSplit` must be retained as an independent cross-check; pdfmake `'2*'` width trap; wiki + shared type JSDoc fixed (#1914, #1917/PR #1994) - [Story reviews](story-reviews.md) — per-story and per-PR review log -- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) — ADR-034 B4 rule + legend addendum landed in PR #1979; per-locale header character budget + "no interface `t` in header/footer" (#1937/#1938, PR #1982); pdfmake `Content` is unspreadable (TS2698) but `Object.assign` needs no cast, and per-item `wordBreak`/newline-only-run facts (#1968, PR #2002). **ADR-034 debt fully PAID 2026-08-04 (#1914)**: width rule #1 (`max(horizontalRatio) <= 1`, not `_minWidth`), module table, override keys, dontBreakRows/height-bound section, injection-only locale contract. `merge.ts` footer/header interface-`t` defect **CLOSED** (footer PR #2000/#1993, header #1938); `TFunction` removed from all of `reportPdf/*` in #2001/PR #2007, so the locale contract is now compiler-enforced there and `buildReportContent.ts` is the single legitimate `TFunction` entry point — ADR-034 lines 82/178/186-188/200/219 all state the old weaker contract and needed a wiki pass (flagged in the PR #2007 review). **ADR-034 rule #1 is WRONG A SECOND TIME (proven in the PR #2008 review, 2026-08-05): `horizontalRatio` is a cell-origin bound, not a content-extent check — it is unconditionally `<= 1` on the all-fixed-width production table and cannot detect token overflow. The check that works is per-cell `_minWidth <= _calcWidth` (the ADR's `wordBreak` false-positive rationale for banning `_minWidth` is empirically false). **Third correction PAID 2026-08-05** (wiki `da1324b`): rule #1 leads with per-cell `_minWidth`, `maxHorizontalRatio` re-scoped to table-box positioning with a vacuity warning, `_minWidth` ban split table-level vs per-cell, 3rd Deviation Log row added. Semantics verified by mutation test in the PR #2008 round-2 review; **two follow-ups still open on the page — every quoted figure (69.28/33.54/266.16pt) is stale, and the rule says "every cell" while the test covers only Usage.** **#1973/PR #2010 (2026-08-05, CHANGES REQUESTED): the 2-hardcoded-shapes era is over — 96 legal column subsets via a single-absorber `computeColumnWidths`, `reportContent/columns.ts` as the AC 2.1 single derivation; `USAGE_WIDTH_7COL` 138.28pt is the NARROWEST Usage width so the 650 chunk budget needed no re-measurement (one-sided clamp). ADR-034's "Geometry constraint that blocks a feature" (line 153) was a false prohibition, plus line 144's constant reference and line 155's "hangs" claim (it throws). **All three PAID by me 2026-08-05, wiki `eb24774`**: section replaced with "Column geometry is a computed engine, not two pinned shapes", `columns.ts` added to the module structure, Deviation Log row. Same commit documented the wizard **tier-factory convention** in Architecture.md (never documented anywhere before, born #1947 after #1943/#1946) incl. the "second `DISCARD_EDITS` opt-out must trigger a tier split" limit.** **#1912/PR #2028 (2026-08-06, APPROVED): required `reportFormatters` is ADR-034 line 230's own "remove the channel" principle, and line 248 already documented the 6-arg signature — no Deviation Log row, but invariant 1 (line 206) now under-claims and wants one sentence on the next ADR-034 pass, alongside a one-liner naming `Formatters` (PDF injection contract) vs `AppFormatters extends Formatters` (app-wide superset). Open medium: `buildReportContent.ts` still has 4 unguarded union-into-`reportT()` keys (lines 143/146/204/274) rendering into the bank PDF — see recurring-patterns "1 of N sites".** +- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) — ADR-034 B4 rule + legend addendum landed in PR #1979; per-locale header character budget + "no interface `t` in header/footer" (#1937/#1938, PR #1982); pdfmake `Content` is unspreadable (TS2698) but `Object.assign` needs no cast, and per-item `wordBreak`/newline-only-run facts (#1968, PR #2002). **ADR-034 debt fully PAID 2026-08-04 (#1914)**: width rule #1 (`max(horizontalRatio) <= 1`, not `_minWidth`), module table, override keys, dontBreakRows/height-bound section, injection-only locale contract. `merge.ts` footer/header interface-`t` defect **CLOSED** (footer PR #2000/#1993, header #1938); `TFunction` removed from all of `reportPdf/*` in #2001/PR #2007, so the locale contract is now compiler-enforced there and `buildReportContent.ts` is the single legitimate `TFunction` entry point — ADR-034 lines 82/178/186-188/200/219 all state the old weaker contract and needed a wiki pass (flagged in the PR #2007 review). **ADR-034 rule #1 is WRONG A SECOND TIME (proven in the PR #2008 review, 2026-08-05): `horizontalRatio` is a cell-origin bound, not a content-extent check — it is unconditionally `<= 1` on the all-fixed-width production table and cannot detect token overflow. The check that works is per-cell `_minWidth <= _calcWidth` (the ADR's `wordBreak` false-positive rationale for banning `_minWidth` is empirically false). **Third correction PAID 2026-08-05** (wiki `da1324b`): rule #1 leads with per-cell `_minWidth`, `maxHorizontalRatio` re-scoped to table-box positioning with a vacuity warning, `_minWidth` ban split table-level vs per-cell, 3rd Deviation Log row added. Semantics verified by mutation test in the PR #2008 round-2 review; **two follow-ups still open on the page — every quoted figure (69.28/33.54/266.16pt) is stale, and the rule says "every cell" while the test covers only Usage.** **#1973/PR #2010 (2026-08-05, CHANGES REQUESTED): the 2-hardcoded-shapes era is over — 96 legal column subsets via a single-absorber `computeColumnWidths`, `reportContent/columns.ts` as the AC 2.1 single derivation; `USAGE_WIDTH_7COL` 138.28pt is the NARROWEST Usage width so the 650 chunk budget needed no re-measurement (one-sided clamp). ADR-034's "Geometry constraint that blocks a feature" (line 153) was a false prohibition, plus line 144's constant reference and line 155's "hangs" claim (it throws). **All three PAID by me 2026-08-05, wiki `eb24774`**: section replaced with "Column geometry is a computed engine, not two pinned shapes", `columns.ts` added to the module structure, Deviation Log row. Same commit documented the wizard **tier-factory convention** in Architecture.md (never documented anywhere before, born #1947 after #1943/#1946) incl. the "second `DISCARD_EDITS` opt-out must trigger a tier split" limit.** **#1912/PR #2028 (2026-08-06, APPROVED): required `reportFormatters` is ADR-034 line 230's own "remove the channel" principle, and line 248 already documented the 6-arg signature — no Deviation Log row, but invariant 1 (line 206) now under-claims and wants one sentence on the next ADR-034 pass, alongside a one-liner naming `Formatters` (PDF injection contract) vs `AppFormatters extends Formatters` (app-wide superset). Open medium: `buildReportContent.ts` still has 4 unguarded union-into-`reportT()` keys (lines 143/146/204/274) rendering into the bank PDF — see recurring-patterns "1 of N sites".** **#1940/PR #2032 (2026-08-06, APPROVED): runt-merge gate + `'… '` continuation marker; the backward-merge induction (receiver virginity is structural) verified by hand AND a 400k-case verbatim-port fuzz; ADR-034 line 152's call-site quote stale a THIRD time and line 148's "bound what a cell renders" rule now under-satisfied by the 2-char unbounded marker (safe by size only: worst case +1 line, 41->42 vs the 44-line budget). #1950 needs no reorder, but its guard pins a rendered quantity that the marker moves to 36 chars / 4 lines / 44.8pt on continuation rows.** - [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 6b2fb74a6..be3b7b88d 100644 --- a/.claude/agent-memory/product-architect/client-pdf-pipeline.md +++ b/.claude/agent-memory/product-architect/client-pdf-pipeline.md @@ -584,3 +584,59 @@ while `realRender.test.ts` covers only Usage — Vendor at 45pt is the binding c lists over the same union, and the tests that "count" them pin the literals 7/6, so a new key is silently absent everywhere. Recommended fix shape: derive the canonical order from an exhaustive `Record` and define the base sets as filters over it. + +## #1940 / PR #2032 — runt-merge + continuation marker (reviewed 2026-08-06, APPROVED) + +`packUsageCellRowsWithMinimum(segments, maxChars, minTrailingChars)` wraps `packUsageCellRows` +(both `packUsageCellRows` and `splitIntoPageSafeChunks` byte-identical to `beta`, hash-verified). +Production floor: `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS /* 20 */, usageSafeTokenChars)`. +Render-time marker: a bare `{ text: '… ' }` run prepended by `buildUsageCell(segments, true)` for +`packedCellRows` index >= 1 — never in `UsageCellSegment.text`, so I1 stays trivially true. + +**The AC1-vs-AC2 resolution worth reusing.** Three candidate designs; only one works: +merge-without-repack is *unsound* (receiver can already be at `maxChars`); always-repack-at-reduced +budget is sound but regresses the zero-degradation range; **gate the reduced-budget repack behind an +actual runt check** is the only one that pays neither. A "lookahead inside the packer" is not a +single-pass alternative — you cannot know a remainder exists without packing to the end, so the +lookahead *is* the first pass, and folding it in would cost the primitive its clean +"every row <= the budget I was given" contract, which is what the safety proof rests on. + +**The backward-merge induction, stated so it can be defended.** (1) primitive guarantees +`rowCharCount <= B`; (2) **receiver virginity is structural**: at counter `i` the only index written +is `i-1`, the counter strictly decreases, so index `k` is written iff the counter equals `k+1` — +exactly once ever; `splice(i,1)` shifts only indices `> i`; (3) the donor is `< min` *at the moment +of donation* because the guard re-reads the possibly-already-grown row. Hence +`(<= M-m) + (< m) < M` at any cascade depth. Verified by hand **and** by a 400k-case fuzz +(verbatim ports, `maxChars` 2..61, `min` 1..maxChars+4 incl. the degenerate band, meta segments, +leading empty segments): 0 AC1 / 0 AC2 / 0 I1 violations. **Fuzzing verbatim function ports in a +throwaway `.mjs` is the cheapest independent read of an induction argument — do this again whenever +a doc comment carries a proof.** + +**Wrapping row-level output was the right layer, for a stronger reason than the PR gave.** The PR's +reason (a runt arises from two paths, one of which the chunker never sees) is true — the packer's +own `used > 0 && rest.length <= maxChars` flush creates runts with `splitIntoPageSafeChunks` never +invoked. But the load-bearing reason is a **unit mismatch**: the AC's unit is the rendered *row*; +the chunker's unit is a chunk within one *segment*. A row can hold a prose chunk AND the grey meta +segment, so a chunk-level floor bounds the wrong quantity. + +**Findings left open (all non-blocking, for the ADR-034 pass):** +- ADR-034 **line 152**'s call-site quote is stale a **third** time (`MAX_SAFE_USAGE_CHUNK_CHARS` -> + `usageChunkChars` -> now `packUsageCellRowsWithMinimum(..., minTrailingUsageChars)`). Three + staleness events on one quoted signature: name the *contract*, drop the literal call. +- ADR-034 **line 148**'s rule ("bound what a cell *renders*") is now literally under-satisfied — the + marker is rendered and unbounded. Safe *by size only*: 2 chars, worst case **+1 line** (when the + next token is <= 16 chars so no `break-all`, but too long for the 14 slots left beside `'… '`); + 0 lines in the break-all case. 41 -> 42 vs the 44-line `№` budget. Nobody wrote that down, and the + ux spec already budgets 14 chars for a *textual* marker variant — the obvious next request. +- `packUsageCellRows` now has **exactly one production caller** (the wrapper). Add a "production + callers go through the wrapper" line to its doc comment. +- ux-designer's "threshold-to-ceiling **ratio** stays roughly constant across subsets" is **false** + (the one-sided clamp pins `usageChunkChars` at 650 while `usageSafeTokenCharsForWidth` scales with + width: ratio runs ~3% -> ~9.5%). Harmless — the algebraic bound is subset-independent — but do not + let the ratio framing get copied into the ADR as the reason. + +**#1950 sequencing: confirmed no reorder needed.** No geometry constant moves, and the repack budget +is strictly *below* the ceiling, so the new consumer is more conservative. But #1950's guard pins a +**rendered** quantity, and the marker adds 2 uncounted rendered characters: on a continuation row the +real overage against the derived `Ѹ` 616 ceiling is 36 chars / **4 lines / 44.8pt**, not 34 / 3 / +33.6. State which quantity the guard pins when #1950 lands. diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index bc4fcfd86..239296506 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -1462,3 +1462,31 @@ return interface (proves nothing depended on the structural-only relation); `com first declaration in the rule and the composed class must not share properties with the composer (source order decides, both being single-class selectors); grep the *old* CSS-module class name across `e2e/` — a POM `[class*="step4Body"]` locator survives a rename as a zero-match locator with Jest green. + +## Fuzz the verbatim ports when a doc comment carries a proof (#1940, PR #2032) + +When an AC's whole correctness rests on an induction argument written in a doc comment, a hand-trace +(mine, plus the dev-team-lead's) is two reads of the same reasoning, not two independent checks. +Copy the functions verbatim into a throwaway `.mjs` and fuzz the *stated postconditions* across a +parameter space that includes the degenerate guards — 400k cases took under a minute and covered +the cascade, the mid-list runt, the hard-split path, and the meta-segment boundary at once. + +**Why:** a hand-trace confirms the argument the author wrote; it does not search for the case the +author did not consider. Only randomized inputs do that. +**How to apply:** any PR whose doc comment says "and this bound holds at any depth / for all N". +Write the ports, assert the postconditions, `rm` the file before committing. Note that the harness +must be created with `Write` (the Bash tool refuses heredoc redirects inside a worktree session). + +## A safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940) + +The #1940 ux spec argued the merge stays safe across all 96 subsets because "the threshold-to-ceiling +ratio stays roughly constant." False: `usageChunkCharsForWidth`'s **one-sided clamp** pins the +numerator's ceiling at 650 for every subset while `usageSafeTokenCharsForWidth` scales linearly with +width — the ratio runs ~3% to ~9.5%. The implementation was safe anyway (its bound is algebraic and +subset-independent), so this never became a defect — but the ratio sentence was on its way into a +code comment and the ADR. + +**Why:** proportional-scaling arguments are the first thing a clamp, a floor, or a `Math.max` breaks, +and they are exactly the arguments that read as obviously true. +**How to apply:** whenever a spec or comment justifies safety by "both sides scale off the same +basis", grep the chain for `Math.min`/`Math.max`/`Math.floor` before letting the sentence land. diff --git a/.claude/agent-memory/ux-designer/MEMORY.md b/.claude/agent-memory/ux-designer/MEMORY.md index 754bfa7b8..5565a349a 100644 --- a/.claude/agent-memory/ux-designer/MEMORY.md +++ b/.claude/agent-memory/ux-designer/MEMORY.md @@ -12,7 +12,7 @@ - [feature-spec-history.md](feature-spec-history.md) — detailed notes from past visual specs posted to GitHub issues, by story number - [pr-review-findings.md](pr-review-findings.md) — past PR design-review findings, verdicts, and recurring bugs; process notes for posting GitHub reviews/comments - `story-4-9-invoice-linking-hi.md`, `pr-1490-measurement-freehand.md`, `annotator-a11y-audit.md` — standalone detailed reports referenced from the files above -- [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md) — render-and-rasterize technique for `reportPdf/` reviews; PR #1935/#1929 rounds 1-4, both APPROVED (word-break-without-hyphen, page-1-top-margin, "test the common case not just the adversarial one" lessons) +- [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md) — render-and-rasterize technique for `reportPdf/` reviews; PR #1935/#1929 rounds 1-4, both APPROVED (word-break-without-hyphen, page-1-top-margin, "test the common case not just the adversarial one" lessons); PR #2032/#1940 APPROVED (runt-merge + ink-only continuation marker, cross-subset render check via `hiddenColumns`) ## Quick-reference rules (apply on every spec/review) diff --git a/.claude/agent-memory/ux-designer/pdfmake-rendering-verification.md b/.claude/agent-memory/ux-designer/pdfmake-rendering-verification.md index 878d47beb..3f2ef7dad 100644 --- a/.claude/agent-memory/ux-designer/pdfmake-rendering-verification.md +++ b/.claude/agent-memory/ux-designer/pdfmake-rendering-verification.md @@ -51,6 +51,37 @@ Design-review implication: don't take an AC like "rows are not split across page just because the layout code sets the documented pdfmake flag and the existing tests pass — for a `dontBreakRows` claim specifically, render-and-inspect before approving. +## #1940 closed (PR #2032): runt-merge + continuation marker, verified at both column-subset extremes + +The round-4 "single stray character" degenerate row (above) got its own issue and PR. +`packUsageCellRowsWithMinimum` wraps `packUsageCellRows` with a backward-merge pass so no +continuation row (index >= 1) ever renders under `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, +usageSafeTokenChars)` characters — hand-traced the backward `for` loop against the exact +`'z'.repeat(2*maxChars+1)` repro and it holds at any cascade depth (a runt merged into a row that +becomes a runt itself gets re-checked, since the scan re-tests the receiver before moving on). +`buildUsageCell(segments, isContinuation)` prepends a literal `{ text: '… ' }` run (no `color`/ +`bold`/`fontSize` override — deliberately ink-only for greyscale/photocopy robustness) as the +FIRST run when `isContinuation`, and the cell still returns `{ text: runs, style: 'tableCell' }` +so the marker inherits the same 8pt non-bold body style as everything else — checked the actual +run object, not just "a marker exists somewhere in the cell." + +**New technique validated here**: to check a per-cell visual signal across #1973's column-subset +range, pass a `hiddenColumns` Set into `buildOverviewContent(content, skipped, hiddenColumns)` to +force the narrowest legal subset (`{allocatedAmount, usage}` — hide vendor/invoiceNumber/date/ +status/invoiceAmount) alongside the default widest (7-col, empty Set), render both through the +real pipeline, and rasterize both. This is the right check whenever a spec says a signal must +"read correctly" across subsets, not just render without crashing — a marker/badge can be +structurally present at every width and still be illegible or ambiguous at the narrow end, and a +single-subset render can't catch that. Confirmed for the '… ' marker: legible and unambiguous at +both the 2-column and 7-column shapes, including the 2-column case where Usage is the only +non-blank cell on a continuation row at all. + +Also confirmed: the font the glyph-coverage check (`fontkit.glyphForCodePoint(0x2026)`) targets +(`Roboto-Regular.ttf`, per `loader.ts`'s `normal:` mapping) is the SAME font `tableCell` actually +renders with (non-bold body style) — worth checking this alignment explicitly whenever a PR adds a +glyph-coverage check for a specific font file, since checking the wrong font/weight file would +pass while the actually-rendered glyph could still be `.notdef`. + ## Round 3 (PR #1935, same issue): fix confirmed, root cause was the wrong object Round 2 moved `dontBreakRows` onto `table.dontBreakRows` (the object `TableProcessor.js:123`