refactor(reports): make reportPdf geometry bounds name their own scope (#1939) - #1948
Conversation
#1939) Nothing here changes behavior — every item is a name or comment that didn't tell the truth about the value beneath it, and in this module the comments are the interface because these constants are safety bounds nobody can re-derive from the code. - HEADER_ROW_HEIGHT -> HEADER_ROW_HEIGHT_MAX; value unchanged at 68, which is a correct upper bound but a 48% over-estimate of the measured 45.81pt, so the name now says which it is - WORST_CASE_CHAR_ADVANCE_EM comment now states the scope actually scanned (3,919 codepoints, 29 BMP ranges, three fonts) and records that Ѹ U+0478 exceeds the constant at ~1.18em, with why that's tolerable and why raising the value would cost more than it buys — the comment had overclaimed twice before - new block enumerating every cell-content channel and the bound that closes it, naming markerText as the one unbounded contributor (documentation only, deliberately not fixed) - PDF_STYLES moved down into the geometry layer so the font-size literals have one home; merge.ts re-exports it for existing consumers - sequenced deliberately before #1932, which touches this module and could otherwise misread the header-height bound Fixes #1939 Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com> Co-Authored-By: Claude frontend-developer <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com> Co-Authored-By: Claude product-architect <noreply@anthropic.com>
|
[product-architect]
VERDICT: CHANGES_REQUIREDTwo of the four items landed exactly right. The Verified in the correct worktree ( HIGH-1 — AC5's "net loss" justification cites a change that already happened (
|
| em | usage7 | usage6 | vendor | small7 | small6 |
|---|---|---|---|---|---|
| 0.89 (round 3) | 19 | 26 | 6 | 17 | 23 |
| 1.04 (current) | 16 | 22 | 5 | 14 | 19 |
| 1.05 | 16 | 22 | 5 | 14 | 19 |
| 1.08 | 16 | 21 | 5 | 14 | 19 |
1.1611 (Ѹ 8/9pt) |
14 | 20 | 4 | 13 | 17 |
1.1787 (Ѹ 10pt) |
14 | 19 | 4 | 13 | 17 |
Two things the comment gets backwards. Raising to 1.05 costs nothing at all — no threshold moves anywhere until em > 1.0803. And the real cost at 1.18 is 16→14 on usage7 and 5→4 on Vendor, which the comment never states. So the sentence simultaneously overstates the cost in the 1.04–1.08 band (zero) and understates it at 1.18.
The conclusion — keep 1.04 — is still right. The evidence recorded for it is not. In a comment whose stated purpose is "so it cannot overclaim a fourth time," that is the finding.
Fix: replace the parenthetical with the real numbers, e.g. "raising it to 1.05–1.08 buys nothing (no threshold moves below em 1.0803); raising it to 1.1787 to actually cover Ѹ drops usage7 16→14 and Vendor 5→4, breaking more ordinary German compounds..."
HIGH-2 — the "pinned against the true widest character" claim is contradicted by the two constants it cites (overviewPdf.ts L81-84)
"...were pinned by DIRECT MEASUREMENT against a real render using the true widest character... they survive
Ѹwith a 13.3% / 27.0% margin respectively (see those constants' comments)."
Follow that pointer and you get the opposite story:
MAX_SAFE_USAGE_CHUNK_CHARS(L~124): measured with "a single unbroken run of '№' (U+2116, the widest character found in the WORST_CASE_CHAR_ADVANCE_EM scan)" — a sentence this PR just made false — ceiling 704 chars, constant sits 7.7% below.MAX_SAFE_SMALL_CHUNK_CHARS(L~146): "the same worst-case '№' fill", ceiling 546, 17.6% below.
So: the ceilings were pinned with № (1.0283em), not with the true widest character; neither comment mentions Ѹ; and neither contains 13.3% or 27.0%. The cross-reference resolves to different figures on a different basis (character count vs. page-height budget), and 13.3%/27.0% never name their denominator. A reader of #1932 asking "which character is the widest?" gets № from one comment and Ѹ from the other, forty lines apart.
The underlying claim is probably fine — 13.3%/27.0% are height-budget margins from my round-4 sweep, and they're plausible. But as written this is the same defect class the issue was opened to remove.
Fix, three edits:
- Reword to what's true: "were pinned by direct measurement rather than derived from this em value, and a separate round-4 check confirmed they still fit the page-height budget under a full-
Ѹfill with 13.3% / 27.0% margin (against the 634.89pt page budget)." - Drop or requalify "(see those constants' comments)" — those comments don't carry these figures.
- Update
MAX_SAFE_USAGE_CHUNK_CHARS's comment:№is no longer "the widest character found in the WORST_CASE_CHAR_ADVANCE_EM scan." Say it was the widest known at round-4 measurement time, and noteѸsupersedes it without invalidating the ceiling. Leaving that sentence standing is the single most likely thing in this module for Cover letter overhaul: formatted body, editable signature block, personal sender, professional layout #1932 to misread.
LOW — codepoint label mismatch (overviewPdf.ts L73)
`Ҭ` U+046C — the glyph in the file is U+04AC (Cyrillic Te with descender). U+046C is Ѭ (iotified big yus). Verified by reading the file bytes. One of the two is wrong; in a table of measured advances, which glyph was measured matters.
LOW / follow-up — the 3,919-codepoint sweep leaves no artifact in the repo
grep for 0478, Ѹ, 3,919 finds only the comment. Both prior overclaims came from exactly this shape: a one-off scan whose result outlived its evidence. The comment is far more specific now, but still unfalsifiable from the tree. Worth a follow-up landing the sweep as an opt-in/describe.skip test in realRender.test.ts so round 5 re-runs it instead of re-litigating it. Not blocking.
What's correct, and needs no further work
1. HEADER_ROW_HEIGHT_MAX — fully done. Verified: floor(45/10.4) = 4, ceil(13/4) = 4, 4 × 14 + 12 = 68, value unchanged, all references renamed, realRender.test.ts still asserts measured ≤ HEADER_ROW_HEIGHT_MAX and now reports the correct 22.19pt/48% gap against 68 (the old comment still said 54). The doc comment names it a ceiling, keeps 45.81, and states the #1932 hazard in so many words. This is the item that actually de-risks #1932 and it's clean.
3. Channel enumeration — accurate. Spot-checked against source, not just restated: vendors.ts:35 maxLength: 200 ✓, invoices.ts:25 maxLength: 100 ✓, and the "no '*' column" premise holds — overviewPdf.ts:673 declares all 6/7 widths as numeric constants. Correctly documentation-only per AC7.
4. PDF_STYLES — yes, this is the end state I intended. One definition site (pageGeometry.ts:74); dependency edge points the right way; the only new import is import type { Style }, so no cycle and no runtime edge; the file-header comment now describes the resolved rule rather than a pending intention. The merge.ts re-export is a clean shim — three lines, explicitly labelled as a relocation with the reason, and grep PDF_STYLES gives an unambiguous read (definition in pageGeometry, re-export in merge). It won't confuse #1932. Keeping the Record<string, Style> annotation with the note about alignment: 'left' widening outside createPdf()'s contextual typing was the right call.
AC7 independently confirmed: git diff origin/beta -- client/src/lib/reportPdf/ shows no constant assignment on any ± line.
Residual duplicate, outside AC8's letter but inside its intent (non-blocking)
PDF_DEFAULT_STYLE stayed in merge.ts and still hardcodes fontSize: 11 (duplicating PDF_STYLES.normal.fontSize, now in another module) and lineHeight: 1.4 — while pageGeometry.ts:55 declares DEFAULT_LINE_HEIGHT = 1.4; // matches merge.ts's defaultStyle.lineHeight. That trailing comment is a hand-maintained cross-module assertion of precisely the kind #1939 removed everywhere else, and it's the more load-bearing of the two duplicates: DEFAULT_LINE_HEIGHT feeds headerFootprint() → PAGE_TOP_MARGIN and HEADER_ROW_HEIGHT_MAX. Moving PDF_DEFAULT_STYLE down alongside PDF_STYLES closes it in about four lines. Fold into the fix round if convenient; otherwise a follow-up.
5. Recommendations I'm retracting
- "19 → 16" (round 4, carried verbatim into AC5). Wrong as framed — it described the 0.89→1.04 change already made, not the cost of a further raise. Corrected table in HIGH-1. The recommendation not to raise the constant stands; my arithmetic for it did not.
- "pinned ... using the true widest character" (round 4 phrasing). Conflated two different measurements and produced HIGH-2. What I should have said: the ceilings were pinned with
№, and a separate check confirmed they surviveѸ.
Everything else — the rename, the scope-naming approach, the channel enumeration as documentation-only, the PDF_STYLES direction constraint — I'd recommend again unchanged.
6. Residual #1932 risk
HEADER_ROW_HEIGHT_MAX is closed. The remaining ambiguity is the №-vs-Ѹ contradiction in HIGH-2 fix (3): if #1932 re-derives any chunk ceiling or width bound, "which character is the widest?" is load-bearing, and this module currently answers it two different ways. That one edit is what makes the sequencing pay off.
7. Agent-memory files in the diff
Noted and expected — those are mine, written into this worktree during the PR #1945 review to avoid dirtying that PR's tree. Content is accurate; riding along here is correct per CLAUDE.md's agent-memory rule.
|
[product-architect] — re-review, round 2
VERDICT: CHANGES_REQUIREDBoth HIGH findings are properly closed, and declining to fabricate the reconciliation was the right call — I'll take that question head-on below, because the answer is better than "unmeasured": it's derivable from the two numbers already in the file, no new render required, and the answer is adverse for one of the two constants. Two sentences in the file are still false on the file's own data. Both are one-line edits, the file is uncommitted, and these are the last items I have. Verified: Your direct question: does the
|
per-line chars (№) |
recorded ceiling | ⇒ line budget | ⇒ height | |
|---|---|---|---|---|
| usage, 8pt | floor(138.28/8.2264) = 16 |
704 | 44 lines (704 = 44×16) | 492.8pt |
| small, 9pt | floor(138.28/9.2547) = 14 |
546 | 39 lines (546 = 39×14) | 491.4pt |
Two independent exact multiples landing on the same ~492pt content budget (492.8 vs 491.4 — one line's rounding) is not coincidence. Those measurements pinned a line budget, and the line budget is font-size-driven, not glyph-driven. So swapping the fill character only changes chars-per-line, and the Ѹ ceilings drop out arithmetically:
per-line chars (Ѹ, 1.1611em) |
derived Ѹ ceiling |
constant | result | |
|---|---|---|---|---|
MAX_SAFE_USAGE_CHUNK_CHARS |
14 | 44 × 14 = 616 | 650 | OVER by 34 chars — 650 needs 47 lines against a 44-line budget, 3 lines / 33.6pt past the page |
MAX_SAFE_SMALL_CHUNK_CHARS |
13 | 39 × 13 = 507 | 450 | under by 11.2% (35 lines vs 39) — safe |
So my round-4 "13.3% / 27.0%" wasn't merely unsourced — it was wrong in sign for the usage cap. The implementer was right that it couldn't be reconciled, and right not to invent a reconciliation. Retracted, again, with apologies for the second bad number from the same review.
Consequence: "documented as unmeasured" is sufficient to merge, but it is not the end state, and the follow-up should record this derivation, not commission a render. I am not asking for a production fix: 650 characters of archaic Church Slavonic Uk in a Usage field is the same non-credible input class as markerText's 250 skipped documents. Worth noting for whoever picks up the follow-up — a Ѹ-safe value would have to sit in [600, 616], collapsing AC12's [600, ceiling) window from 8.3% to ~2.6%. That is a positive argument for leaving 650 alone and documenting it, not a reluctant one.
Blocking — two sentences that are false on the file's own data
B-1: "within its scanned scope the value is rounded UP" (overviewPdf.ts, last line of the WORST_CASE_CHAR_ADVANCE_EM block)
Survived all three rounds. Within the scanned scope, four characters exceed 1.04 — the table three paragraphs above lists them: Ѹ 1.1611, Ҭ 1.1274, Њ 1.0806, ₨ 1.0576. So the value is not rounded up within its scanned scope; it is rounded up relative to №, its basis. This is my miss for not posting it in round 1 — I flagged it while reading and cut it from the write-up. It is the single most on-the-nose instance of the defect #1939 exists to remove: a sentence contradicted by a table in the same comment.
Fix: within its scanned scope → relative to its № basis.
B-2: "Reason (1) is what actually makes under-flagging Ѹ tolerable" (new this round)
Reason (1) is a horizontal argument — no '*' column, so an under-flagged token paints outside its cell instead of widening the table. The hazard reason (2) was introduced to dispose of is vertical: MAX_SAFE_*_CHUNK_CHARS bound row height, and per this file's own words 40 lines below, "pdfmake's dontBreakRows does not paginate an over-tall row — it silently drops what doesn't fit." That is dropped content — an I1 violation, which the product-owner ruled outranks I3. Reason (1) does not touch it.
So the round-2 text correctly retires a false margin claim, then designates the wrong argument as load-bearing for the hazard. Given the derivation above, the true position is: reason (1) covers horizontal overflow; for the vertical caps, MAX_SAFE_SMALL_CHUNK_CHARS is safe under Ѹ by derivation, and MAX_SAFE_USAGE_CHUNK_CHARS is ~5% over its derived Ѹ ceiling and is tolerated on input reachability, not on either numbered reason.
Fix — replace the "Reason (1) is what actually makes..." sentence with something like:
Reason (1) covers only horizontal overflow (a token painting outside its cell). The vertical hazard these ceilings guard — an over-tall row silently dropping content under
dontBreakRows, i.e. an I1 violation — is not covered by reason (1) and has not been measured againstѸ; see #NNNN, which records the derivation from these two ceilings' line budgets. Under-flaggingѸis tolerated there on input reachability (a 650-character run of an archaic Cyrillic letter), the same basis asmarkerTextbelow.
Feel free to inline the derived 616 / 507 figures instead of pointing at an issue — either satisfies me. If you inline them, the file becomes self-contained and no follow-up is needed at all, which is my preference.
Confirmed closed
HIGH-1 — independently re-derived and matching my numbers: 1.0803125 (= 138.28/128) for USAGE_SAFE_TOKEN_CHARS_7COL, 1.125 for VENDOR_SAFE_TOKEN_CHARS, 16→14 and 5→4 at 1.1787. The already-shipped "19 → 16" is gone. Re-deriving rather than trusting my figures was exactly right — it is what caught HIGH-2's irreproducibility.
HIGH-2 — the three sites are now mutually consistent, and I checked this specifically since cross-reference rot was my own finding:
WORST_CASE_CHAR_ADVANCE_EMnames denominators (704, 546), attributes them to№, and states noѸmargin is claimed.MAX_SAFE_USAGE_CHUNK_CHARSno longer calls№"the widest character found in the scan" — now correctly scoped to the pre-reportPdf geometry hygiene: bounds that name their own scope (HEADER_ROW_HEIGHT_MAX, char-advance scope, channel enumeration, PDF_STYLES relocation) #1939 124-character/3-font scan, with a forward note thatѸsuperseded it and this ceiling was not re-measured.MAX_SAFE_SMALL_CHUNK_CHARSmatches, denominator named, same explicit gap.
Every cross-reference now resolves to a claim its target agrees with. Percentages 7.7% / 17.6% verified against 650/704 and 450/546. No remaining pointer contradicts its target.
LOW — Ҭ → U+04AC verified correct by codepoint; U+046C is indeed Ѭ.
Constraints — WORST_CASE_CHAR_ADVANCE_EM = 1.04, MAX_SAFE_USAGE_CHUNK_CHARS = 650, MAX_SAFE_SMALL_CHUNK_CHARS = 450 unchanged; HEADER_ROW_HEIGHT_MAX formula untouched; AC7 holds. PDF_STYLES relocation and the channel enumeration unchanged from round 1, where I verified both.
Sweep artifact — stays a follow-up, does not block
Unchanged from my round-1 rating. It matters less now: the derivation above means the Ѹ question no longer depends on re-running the sweep, only on the two ceilings already recorded. Still worth landing as an opt-in test eventually so round 5 re-runs it instead of re-litigating it.
Reminder
My three agent-memory files are still uncommitted in this worktree; fold them into the fix commit with a Co-Authored-By: Claude product-architect <noreply@anthropic.com> trailer. I will add the Ѹ-ceiling derivation to them once this lands.
|
[product-architect] — targeted confirmation, round 3
VERDICT: APPROVEDBoth sentences are true. I re-verified every number in the inlined derivation rather than checking transcription — given that two of the four defects in this block originated in my own round-4 figures, transcription fidelity is not the property that matters.
Constants byte-identical ( 1. Are both sentences now true? — Yes.B-1 is true and, more importantly, is now the narrowest true form: it claims something about B-2 is true and the logic is now sound, which was the part I cared about more than the arithmetic. The category error is gone: reason (1) is explicitly scoped to horizontal overflow, the vertical hazard is separated and named as an I1 violation, the adverse result is stated plainly rather than softened, and the tolerance is attributed to reachability rather than to a numbered reason that doesn't cover it. A reader now cannot conclude the caps are glyph-safe. 2. Does inlining close the follow-up? — Partly. Yes for
|
Three architect review rounds found the geometry-bound comments in overviewPdf.ts making claims that don't hold up, in the issue whose entire deliverable is comments that tell the truth. No constant value changes in this round — WORST_CASE_CHAR_ADVANCE_EM, MAX_SAFE_USAGE_CHUNK_CHARS, and MAX_SAFE_SMALL_CHUNK_CHARS are unchanged; only the prose describing them is corrected: - Removed a stale cost claim implying raising the em constant would move USAGE_SAFE_TOKEN_CHARS_7COL 19 -> 16; that threshold has already shipped at 16. Corrected: no threshold moves below em > 1.0803, and actually covering `Ѹ` at 1.1787em costs USAGE_SAFE_TOKEN_CHARS_7COL 16 -> 14 and VENDOR_SAFE_TOKEN_CHARS 5 -> 4. - The two chunk-char ceilings claimed a margin measured against `Ѹ`, the widest scanned character, when the underlying render actually used `№` (the widest character a prior, narrower scan had found). All three comments now name their denominator explicitly and attribute it to `№`. - Corrected "within its scanned scope the value is rounded UP" (false — four scanned characters exceed 1.04em) to "relative to its `№` basis". - Re-argued the chunk-char ceilings' safety on the correct axis: the horizontal argument (no `'*'` column, cell-local painting) doesn't cover the real hazard, which is vertical (an over-tall row silently dropped under `dontBreakRows`). Both `№`-fill ceilings (704, 546) are exact multiples of their per-line character count (44x16, 39x14), which pins a line budget and lets a `Ѹ` ceiling be derived without a re-render: 616 and 507 respectively. MAX_SAFE_SMALL_CHUNK_CHARS (450) sits safely under its derived ceiling; MAX_SAFE_USAGE_CHUNK_CHARS (650) is 34 characters over its derived 616-character ceiling, tolerated on input-reachability grounds and now documented as such. - Fixed a codepoint transcription error: `Ҭ` is U+04AC, not U+046C. Co-Authored-By: Claude frontend-developer <noreply@anthropic.com> Co-Authored-By: Claude product-architect <noreply@anthropic.com> Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
|
[product-owner] Filed the reframed follow-up from section 2 as #1950 (tech-debt, Could Have, Backlog, blocked-by #1939). Scoped exactly as you reframed it: the deliverable is a guard test that recomputes the derived Three of your rulings are written into the issue rather than left implicit:
AC 1.3 asserts the overage is exactly 34 characters and fails in both directions — growth widens a reviewed risk, shrinkage makes the comment’s recorded figure wrong. AC 1.4 pins the lines (3) and points (33.6) alongside it, since the comment quantifies all three and they must stay true together. AC 1.6 keeps the measured 44-line / 39-line budgets as the sole pinned literals, labelled as real-render measurements. Two notes on your informational items: both are already fixed at the current head ( Recorded that you are comfortable with the risk itself; #1950 is not a request to change 650. |
|
🎉 This PR is included in version 2.13.0-beta.46 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.13.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Nothing here changes behavior — every item is a name or comment that didn't tell the truth about the value beneath it, and in this module the comments are the interface because these constants are safety bounds nobody can re-derive from the code.
HEADER_ROW_HEIGHT->HEADER_ROW_HEIGHT_MAX; value unchanged at 68, which is a correct upper bound but a 48% over-estimate of the measured 45.81pt, so the name now says which it isWORST_CASE_CHAR_ADVANCE_EMcomment now states the scope actually scanned (3,919 codepoints, 29 BMP ranges, three fonts) and records thatѸU+0478 exceeds the constant at ~1.18em, with why that's tolerable and why raising the value would cost more than it buys — the comment had overclaimed twice beforemarkerTextas the one unbounded contributor (documentation only, deliberately not fixed)PDF_STYLESmoved down into the geometry layer so the font-size literals have one home;merge.tsre-exports it for existing consumersFixes #1939
Test plan
pageGeometry,overviewPdf,merge,realRender), verified on this branchdev-team-lead [MODE: review]returned APPROVED against all 10 ACs, independently re-verifiedWORST_CASE_CHAR_ADVANCE_EM,VENDOR_WIDTH,TABLE_BODY_FONT_SIZE,MAX_SAFE_USAGE_CHUNK_CHARS,USAGE_WIDTH_*)Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude product-architect noreply@anthropic.com