feat(export): auto-fit body size and swap the embedded font to Liberation Sans - #879
feat(export): auto-fit body size and swap the embedded font to Liberation Sans#879s-annam wants to merge 1 commit into
Conversation
…tion Sans (#878) The Download-PDF export set every résumé at a fixed 8.5pt in Poppins. Poppins is a geometric display face doing text-face work — measured at 1.14x the width of a normal text sans, so the shrinking done to fit a page was paying for the typeface rather than for the content, and Liberation Sans at 9.5pt is still narrower than Poppins at 8.5pt. Swap to Liberation Sans: a metric-compatible Arial clone under the SIL OFL, 663 code points of Latin-Extended against Poppins' 467, and a 1.117em line box against Poppins' 1.40em. The built-in PDF fonts are not an option here — they encode WinAnsi only, so a candidate named Wiśniewska degrades to "?" and findExportGlyphLosses refuses the download outright (#664). With the font paid for, one fixed size stops making sense. Express the whole scale at one reference size and multiply it (makeTypeScale), then have the render walk FIT_LADDER = [10, 9.5, 9, 8.5, 8] descending and keep the largest rung achieving the fewest pages. Across the 60-fixture corpus 56 now draw at 10pt — every one of them was previously set a point and a half smaller than it needed to be. Page height scales as size^1.76 rather than size^2 (wrap counts are integers), so absorbable overflow is (base/floor)^1.76: 1.48 pages from a 10pt base, but only 1.08 from the old 8.5pt one. The base was the interesting parameter, not the floor. Gaps, rule and name block are ~19% of the usable page, so they scale with the body — body-only scaling was measured and loses about a third of the achievable shrink. The pass that picks the size is the pass that makes the bytes, so the decision can never be made against geometry the download does not have; it costs ~36ms mean per résumé because 56/60 exit on rung one. The new freedom exposed two layout defects, both fixed here. A wrapped contact line silently drops fields: the re-parse reads contact off a single header line, so whichever links land on the continuation are lost. fitToOneLine already existed to prevent that but missed by one ulp — the fitted size is exact in the reals and re-multiplies a hair over the content width in binary floating point, which is enough for the wrapper to refuse the row. Shave it, and add the fallback that was missing below the readability floor: break at a field boundary instead of wherever the word wrapper lands, choosing the boundary that lets both rows draw largest, with the links always moving down together. Each row draws with its own linkSpans, since decorateFirstLine only overlays a call's first line — a link on a continuation was losing its clickable annotation as well as its parsed field. A wrapped bullet's tail was indented past its own text: bulletIndent was a scale constant, 14.12pt at the top rung, against a "• " marker only 6.28pt wide. Measure it off the font instead. It stays clear of isWrappedContinuation's markerX + 2 threshold at every rung, so the re-parse still folds a wrapped tail back into the bullet it belongs to. Liberation Sans covers U+2192, which retires both KNOWN_FAILURES rows and takes the round-trip corpus to an empty baselines object; both notes had named this exact condition. The #436 experience-label pin now survives at every rung rather than only at 8.5pt and below, because the one-line role header stopped wrapping. The pagination contracts pin { bodyPt: REFERENCE_BODY_PT }. They build deliberately oversized models and assert where the page broke, so once the engine could resize them they went vacuous rather than red — the fixtures got shrunk until there was no break left to make a claim about. The rung is arbitrary; holding it still is the point. Re-tuning them to green would have encoded the new behaviour into tests written to assert something else. Carlito was evaluated as the alternative and rejected on measurement: its 7.5% narrower advance is optical, since its x-height is 9.5% smaller. Normalized to equal x-height it is 2.1% wider with a 21% taller line box, and at matched x-height on the real renderer it consumed 11% more page for identical wrap counts, at 4.7x the font bytes. Closes #878
Deploying offlinecv with
|
| Latest commit: |
ec0f0ca
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff1e3852.offlinecv.pages.dev |
| Branch Preview URL: | https://feat-export-fit-ladder-issue.offlinecv.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * to name a rung directly. | ||
| */ | ||
| export async function renderAtsResumePdf( | ||
| async function renderAtsResumePdfAtSize( |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
One Blocking finding — a reproducible AC violation in the contact-line field-boundary split — otherwise this is an exceptionally well-documented, well-tested change (6131/6131 tests green, typecheck/lint/build/fixture-PII all clean, and every fail-before claim in the PR's own test plan independently reproduced).
Verdict rule applied: ≥1 Blocking finding → REQUEST_CHANGES. No auto-fix was attempted (Step 5.5 only applies at 0 Blockers).
Note: this verdict is
REQUEST_CHANGESper the rule above (≥1 Blocking finding), but GitHub's API refusesREQUEST_CHANGES/APPROVEfrom a PR's own author ("Review Can not request changes on your own pull request"), so this is posted as aCOMMENTreview instead. Treat it as REQUEST_CHANGES: the Blocking finding above should be fixed before merge.
Blocking
Contact-line field-boundary split can silently produce 3 rows, stranding a field with no separator — the exact defect this PR sets out to eliminate. src/lib/pdf/render-ats-pdf.ts:1826-1836 computes rowSize as Math.min(fitToOneLine(top), fitToOneLine(bottom)) and draws both rows at it without ever verifying either row actually fits on one line at that size. fitToOneLine (Layout.fitToOneLine, line ~561) is floored at CONTACT_MIN_SIZE_RATIO * size (0.8×) — when the true one-line-fit size is below the floor, it still returns the floor value as if the text fit.
Reproduced directly (contact: email + phone + a moderately long location + a "#792 work authorization" statement + 2 links):
jordan.bennett@example.com • (973) 555-0123 • A Truly Extraordinarily Long Municipality Name In The Greater Metropolitan Statistical Area
Region ← orphaned, no separator, no field boundary
Authorized to work without sponsorship in the United States of America • example.com/in/jordan-bennett • example.com/jbennett
3 rows, directly violating AC #878's explicit "at most two rows" guarantee. top's true required size was below the floor, so drawText silently word-wrapped it — the exact "stranded field, no separator, orphaned continuation" defect this whole split mechanism exists to prevent (#425/#664), with zero runtime signal. render-ats-pdf.type-scale.test.ts's sweep ("never strands a field, at any contact length") only pads link length, never identity-field (location/workAuthorization) length, so this path is untested. Not a pathological input — a longer visa-status statement or municipality name is realistic for this product's stated audience.
Fix: verify the split actually worked before trusting it, e.g.:
const rowSize = Math.min(
layout.fitToOneLine(top, layout.t.contact),
layout.fitToOneLine(bottom, layout.t.contact),
);
if (!layout.fitsOneLine(top, rowSize) || !layout.fitsOneLine(bottom, rowSize)) {
// Split didn't help — fall back to the single-line (still-wrapping) draw
// rather than pretend the split succeeded.
layout.drawText(contactLine, { size: fitted, color: muted, linkSpans: [...emailSpans, ...linkSpans] });
} else {
layout.drawText(top, { size: rowSize, color: muted, linkSpans: emailSpans });
layout.drawText(bottom, { size: rowSize, color: muted, linkSpans });
}Secondary
-
contactPartsdropped the.filter(Boolean)safety net for links.src/lib/pdf/render-ats-pdf.ts:1778-1784— old code ran the whole array (identity fields + links) through.filter(Boolean); the new code only filtersidentityParts, then spreadsmodel.contact.linksunfiltered.formatLinkDisplay("https://")→""(strips scheme/www/trailing-slash to nothing) —ats-resume-model.ts'saddLinkgates on the raw URL being truthy, not on the formatted result being non-empty. A degenerate parsed URL reaching this now produces a dangling"• "separator and a zero-widthlinkSpansentry instead of being silently dropped as before. Edge-case trigger, but a real regression of a previously-present defense. -
MARGIN(54→36) andLINE_GAP(1.25→1.15) are undocumented behavior changes outside the issue's stated design.src/lib/pdf/render-ats-pdf.ts:101,160. Issue #878's design section only ever statesMARGIN"stays 36" (never says it's changing from 54) and never mentionsLINE_GAPat all — both are real, page-density-affecting decisions (MARGINis implicitly backed by the issue's own "540.0pt" contact-line example, so it reads as intentional;LINE_GAP's ~8% tighter leading has no measurement or mention anywhere). The PR body doesn't call either out either. Worth a one-line justification in the PR/issue for the historical record — not asking to revert. -
test-setup.ts's new font-serving shim silently breaks under@vitest-environment jsdom.src/test-setup.ts:78-81—fileURLToPath(new URL('./assets/fonts/...', import.meta.url))resolves to/src/assets/fonts/LiberationSans-Regular.ttf(losing the project-root prefix) when the calling test file declares@vitest-environment jsdom, confirmed by direct reproduction (jsdom'sURLimplementation mishandles multi-segmentfile://relative resolution). ThereadFileSyncthen throwsENOENT, caught upstream inloadFonts, and the render silently falls back to Helvetica — exactly the outcome this shim's own docblock says it exists to eliminate ("every layout assertion... was calibrated against a font that was not the shipped one... Restoring the real bytes here is what makes an export test's claim true of the actual download"). Currently dormant —useDownloadPdf.test.tsxis the only jsdom+renderer test today and its assertions don't depend on font geometry — but it's a live trap for the next jsdom test that does. -
render-ats-pdf.flush-right-links.test.ts's localMARGINwasn't updated with production's. Line 16:const MARGIN = 32;(unrelated to production's new36) with a stale// 558comment onRIGHT_EDGE(612-32=580, not 558 — that was true only for the old MARGIN=54). Passes only because of atoBeLessThan(6)tolerance; a real few-point regression in this geometry would go undetected. -
FIT_LADDER's docblock overstatesfitToPage's fallback behavior.src/lib/pdf/render-ats-pdf.ts:194— "When even the floor does not reach one page...fitToPageships the TOP rung rather than the floor" is false in general; the algorithm (correctly, per its own test) ships whichever rung achieves the fewest pages, tie-broken to the largest — that's usually the floor for a genuinely 2+-page résumé, not the top rung. Reproduced: a fixture that's 3 pages at 10pt but 2 pages at 9.5/9/8.5/8pt ships at 9.5pt, not 10pt.fitToPage's own docblock (a few lines below) states the correct, narrower rule ("top rung is the fallback whenever shrinking buys nothing at all"). A future maintainer trusting theFIT_LADDERcomment over the tested behavior could "fix" this into an actual regression.
Nits
- Module docblock still says
~54pt margins(src/lib/pdf/render-ats-pdf.ts:16, unchanged context line, not part of this diff's+lines) thoughMARGINis now 36. multi-experience-roundtrip.test.ts:129's "measured at every rung 8, 8.5, 9, 9.5, 10: 2 labels survive at all five" is asserted only in a comment — the test itself renders via unpinnedfitToPage(a single, whichever-rung-fits render), not a sweep. Independently verified the underlying claim is true at all 5 rungs (2 labels survive at each), so this is a coverage/rigor gap, not a wrong claim — worth afor (const pt of [8, 8.5, 9, 9.5, 10])sweep mirroring the pattern already used elsewhere in this PR (e.g. the contact-length sweep).corpus-roundtripfixtures now pay for up to 5 full render passes per fixture (unpinnedrenderAtsResumePdfcall inroundtrip-hop.ts) inside a{ timeout: 20000 }budget sized for one render. Currently comfortably under budget (full suite: 27.95s, no timeouts) — flagging for awareness as the corpus grows, not asking for a change now.
Gates run
- Issue #878 AC checklist: 9 of 10 checkable ACs verified met by direct reproduction (type-scale identity, top-rung-for-short-résumé, rescue-when-spilling, largest-rung-tie-break,
bodyPtpin/skip, contact sweep non-vacuous, float-boundary fail-before reproduced myself by revertingFIT_ONE_LINE_SHAVE, bullet hanging-indent under text, round-trip corpus green withbaselines: {},#436pin holds at every rung — verified independently). 1 unmet in substance: "at most two rows" for the contact block (Blocking finding above) — the swept AC test passes because it only varies link length, but the underlying contract breaks on identity-field length.npm run verify-equivalent (typecheck + lint + build + fixtures + full test suite) all run locally and green — not literally re-run via theverifyscript name, but every step it chains was executed directly. - 3f description accuracy: PR body's Summary/Test plan are accurate and every checked box was independently reproduced (6131/6131 tests, the boundary fail-before-claim, both U+2192 KNOWN_FAILURES rows retired — plus a third,
extended-latin-name-roundtrip, retired as a side effect of the new font-serving test shim, a bonus not claimed).Review focusquestions 2–4 all check out correctly against the code (verified independently:contactSplitIndexdoes rank on unflooredoneLineSize,bulletIndent's headroom math holds, all 3 pagination-contract render sites are pinned). Question 1 (LINE_GAP) is answered by the Secondary finding above — nothing else inBASEwas left unscaled, butMARGIN/LINE_GAPthemselves are undocumented value changes, not undocumented scaling bugs. - Fixture PII (3a): not applicable — no fixture PDFs/images touched, only sidecar JSON re-bakes matching the issue's stated 3-boolean re-bake exactly.
- Design-system/reuse (3b): not applicable — no
src/components/**touched. - Style tokens (3c): clean — no hardcoded colors/palette classes (grep hits were all
#nnnissue references, not hex colors). - Fallow/dead-code (3d): 0 dead files/exports. One new complexity flag on
renderAtsResumePdfAtSize(199 lines, cyclomatic 16) — report-only per this repo'sverifyconfig, has multiple in-repo consumers and heavy test coverage across 8+ files, not flagging as a finding. - Command-level bugs (3e): only
scripts/fixture-derive.tstouched, comment-only change, no command/flag surface affected.
Head SHA reviewed: ec0f0ca621bc9faa9a52a77bc193fde22177fe11. 👀 was posted at the start of this review.
Reviewed by: Claude Sonnet 5 (high)
| const rowSize = Math.min( | ||
| layout.fitToOneLine(top, layout.t.contact), | ||
| layout.fitToOneLine(bottom, layout.t.contact), | ||
| ); | ||
| layout.drawText(top, { size: rowSize, color: muted, linkSpans: emailSpans }); |
There was a problem hiding this comment.
Blocking — rowSize is trusted without verifying either top or bottom actually fits one line at it. fitToOneLine floors at CONTACT_MIN_SIZE_RATIO, so when the true fit is below the floor this still draws at the floor and drawText silently word-wraps — reproduced with a long location + work-authorization statement: 3 rows, with the overflow word ("Region") stranded on its own line with no separator. Directly violates AC #878's "at most two rows" guarantee. See the review body for a fix suggestion (verify with layout.fitsOneLine before trusting the split).
| model.contact.workAuthorization, | ||
| ...model.contact.links, | ||
| ].filter((p): p is string => Boolean(p)); | ||
| const contactParts = [...identityParts, ...model.contact.links]; |
There was a problem hiding this comment.
Secondary — the old code ran the whole array (identity fields + links) through .filter(Boolean); here only identityParts is filtered, and model.contact.links is spread unfiltered. formatLinkDisplay("https://") → "", and addLink in ats-resume-model.ts gates on the raw URL being truthy, not the formatted result — a degenerate parsed URL can now produce a dangling "• " separator instead of being silently dropped as before.
| const PAGE_WIDTH = 612; // US Letter | ||
| const PAGE_HEIGHT = 792; | ||
| const MARGIN = 54; | ||
| const MARGIN = 36; |
There was a problem hiding this comment.
Secondary — MARGIN 54→36 (and LINE_GAP 1.25→1.15 at line 160) are real, page-density-affecting changes that aren't mentioned in issue #878's design section or this PR's body. MARGIN reads as intentional (the issue's own "540.0pt" contact-line example only works out at MARGIN=36), but neither is called out explicitly — worth a one-line note for the historical record.
| const path = fileURLToPath( | ||
| new URL(`./assets/fonts/LiberationSans-${match[1]}.ttf`, import.meta.url), | ||
| ); | ||
| return new Response(new Uint8Array(readFileSync(path)), { status: 200 }); |
There was a problem hiding this comment.
Secondary — under @vitest-environment jsdom, fileURLToPath(new URL('./assets/fonts/...', import.meta.url)) resolves to /src/assets/fonts/LiberationSans-Regular.ttf (loses the project-root prefix — confirmed by reproduction; jsdom's URL mishandles multi-segment file:// relative resolution). readFileSync then throws ENOENT, caught upstream, and the render silently falls back to Helvetica — exactly what this shim exists to prevent. Dormant today (no jsdom+renderer test asserts geometry yet) but a live trap for the next one.
|
|
||
| const PAGE_WIDTH = 612; | ||
| const MARGIN = 54; | ||
| const MARGIN = 32; |
There was a problem hiding this comment.
Secondary — this local MARGIN wasn't updated to match production's new value (36); RIGHT_EDGE's // 558 comment is also stale (612-32=580, not 558 — that math was only true for the old MARGIN=54). Passes only due to a toBeLessThan(6) tolerance.
| * the x-height is 4.22pt, which is about as small as résumé body text can be | ||
| * set and still survive a recruiter skim on screen. When even the floor does | ||
| * not reach one page the résumé genuinely needs two, and {@link fitToPage} | ||
| * ships the TOP rung rather than the floor — see its docblock. |
There was a problem hiding this comment.
Secondary — this claim ("ships the TOP rung rather than the floor" when nothing fits in one page) is false in general: the algorithm ships whichever rung achieves the fewest pages, tie-broken to the largest — reproduced a fixture that's 3 pages at 10pt but 2 pages at 9.5/9/8.5/8pt, shipping at 9.5pt, not 10pt. fitToPage's own docblock a few lines below states the correct, narrower rule ("fallback whenever shrinking buys nothing at all").
| // company name would put it back at 0 even now. Until #436's title/company | ||
| // disambiguation lands, a change here means the export geometry moved, not | ||
| // that the parser regressed. | ||
| expect(distinctLabels(parse3.canonical.fields.experience ?? []).length).toBe(2); |
There was a problem hiding this comment.
Nit — "measured at every rung... 2 labels survive at all five" is asserted only in this comment; the test itself renders via unpinned fitToPage (one rung, whichever fits), not a sweep. Independently verified the claim is currently true at all 5 rungs, so this is a coverage gap, not a wrong claim — worth sweeping [8, 8.5, 9, 9.5, 10] explicitly, mirroring the pattern already used elsewhere in this PR.
2 participants
Summary
The export set every résumé at a fixed 8.5pt in Poppins — a geometric display face measured at 1.14× the width of a normal text sans, so the shrinking done to fit a page was paying for the typeface rather than the content. Swaps to Liberation Sans (metric-compatible Arial clone, SIL OFL, 663 Latin-Extended code points vs Poppins' 467) and replaces the fixed size with a 10 / 9.5 / 9 / 8.5 / 8 fit ladder that keeps the largest rung achieving the fewest pages. Across the 60-fixture corpus 56 now draw at 10pt — all of them were previously set a point and a half smaller than needed.
The new freedom exposed two layout defects, both fixed here: a contact line that wrapped and silently dropped a parsed field, and a wrapped bullet whose tail was indented ~7.8pt past its own text.
Closes #878
Review focus
src/lib/pdf/render-ats-pdf.ts—makeTypeScalemultiplies oneBASEobject bybodyPt / REFERENCE_BODY_PT. IsLINE_GAPthe only value that must not scale, or did something else sneak intoBASEthat is a ratio rather than a point value?src/lib/pdf/render-ats-pdf.ts—contactSplitIndexscores candidates on the unflooredoneLineSize, notfitToOneLine. Does anything else in the contact path rank candidates through the floored value, where every over-long option scores identically?Layout.bulletIndent— the tail now starts atMARGIN + markerWidth(42.28pt at the top rung, 41.02pt at the floor) againstisWrappedContinuation'smarkerX + 2= 38pt. Is 3pt of headroom at the floor enough, and ismarkerXreallyMARGINon every path that reaches it?src/lib/pdf/export-layout-contract.test.ts— every render site now pins{ bodyPt: REFERENCE_BODY_PT }. Did any pagination assertion get left unpinned, where the fit pass would shrink the fixture until there is no break left to assert?Test plan
npm run typecheckcleannpm run testgreen — 6131 passed, 10 skippednpm run verifyexits 0 (ran in the pre-push hook)baselines: {}— both U+2192 KNOWN_FAILURES rows retiredFIT_ONE_LINE_SHAVEreddens the boundary test)npm run dev— drop a PDF, download the export, check the contact row and a wrapped bullet's tail