fix(xl-pdf-exporter): Keep image aspect ratio, page fit fixes#2887
fix(xl-pdf-exporter): Keep image aspect ratio, page fit fixes#2887Wqrld wants to merge 1 commit into
Conversation
… in PDF export (TypeCellOS#2866) Images in PDF exports were previously given only a fixed width, so oversized bitmaps could distort, overflow the page horizontally, or spill an empty trailing page when taller than the page (images render in an unbreakable group). - Clamp the image box with maxWidth: 100% and a maxHeight cap derived from the page height, page padding, per-block padding, and one reserved caption line. - Use objectFit: contain + objectPosition so a clamped bitmap keeps its aspect ratio and hugs the edge matching the block's textAlignment. - Clamp captions with maxWidth: 100% so they wrap instead of overflowing. - Teach the shared test file resolver to serve placehold.co URLs at the dimensions requested in the URL (patched into the placeholder JPEG's SOF0 marker), failing loudly if the marker is missing. Known limitations (documented in code): multi-line captions and in-flow headers can still make a page-height image group spill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@Wqrld is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds ChangesPDF Image Sizing Fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/xl-pdf-exporter/src/pdf/defaultSchema/blocks.tsx (1)
295-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
exporter: anyparameter defeats the existing type via internal cast.
imageStyletakesexporter: anythen immediately casts it toPDFExporter<any, any, any>internally (Line 300). Typing the parameter asPDFExporter<any, any, any>directly would preserve type-checking onexporter.stylesat call sites and remove the redundant cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/xl-pdf-exporter/src/pdf/defaultSchema/blocks.tsx` around lines 295 - 329, The imageStyle helper currently accepts exporter as any and then immediately casts it to PDFExporter<any, any, any>, which bypasses type safety. Update imageStyle to type the exporter parameter directly as PDFExporter<any, any, any> and remove the internal cast so exporter.styles is checked at the call sites and the redundant any is eliminated.packages/xl-pdf-exporter/src/pdf/pdfExporter.tsx (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManually-synced magic constant risks silent drift.
PAGE_HEIGHTmust be kept in sync by hand with the<Page size="A4">element (Line 270). If that size prop ever changes, this constant silently goes stale andimageStyle'smaxHeightclamp (inblocks.tsx) will be wrong without any compile-time or test signal tying the two together.Consider deriving the page height from a single shared source (e.g. a
PAGE_SIZEobject passed to both<Page size=...>and used to computePAGE_HEIGHT), or at minimum adding a runtime assertion/test that fails if they diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/xl-pdf-exporter/src/pdf/pdfExporter.tsx` around lines 32 - 37, The PAGE_HEIGHT constant in pdfExporter.tsx is manually synchronized with the <Page size="A4"> prop and can drift silently. Update the pdfExporter/toReactPDFDocument flow to use a single shared PAGE_SIZE source for both the <Page> component and PAGE_HEIGHT, or add a runtime/test assertion that verifies the computed height still matches the page size used by toReactPDFDocument and the maxHeight clamp in blocks.tsx.packages/xl-pdf-exporter/src/pdf/pdfExporter.test.tsx (1)
272-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffByte-level regex for page count is a bit brittle but acceptable here.
pdf.match(/\/Type\s*\/Page[^s]/g)).toHaveLength(1)inspects raw PDF bytes rather than parsing PDF structure. It correctly excludes/Type /Pagesvia the negative-lookahead-style[^s], but any future react-pdf output change (e.g. extra whitespace variants, or a/Type /Pageappearing in an unrelated object such as an XObject dictionary) could make this fragile. A PDF-parsing library (e.g.pdf-lib) would give a more robust page count, but that's a heavier lift than warranted here for two tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/xl-pdf-exporter/src/pdf/pdfExporter.test.tsx` around lines 272 - 316, The page-count assertions in these PDFExporter tests rely on matching raw PDF bytes with a brittle regex. Update the two tests in PDFExporter.test.tsx to count pages using a PDF parser such as pdf-lib instead of matching /Type /Page in the buffer, and keep the assertions in the existing image-scaling and caption cases tied to the PDF generated by PDFExporter.toReactPDFDocument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/xl-pdf-exporter/src/pdf/defaultSchema/blocks.tsx`:
- Around line 295-329: The imageStyle helper currently accepts exporter as any
and then immediately casts it to PDFExporter<any, any, any>, which bypasses type
safety. Update imageStyle to type the exporter parameter directly as
PDFExporter<any, any, any> and remove the internal cast so exporter.styles is
checked at the call sites and the redundant any is eliminated.
In `@packages/xl-pdf-exporter/src/pdf/pdfExporter.test.tsx`:
- Around line 272-316: The page-count assertions in these PDFExporter tests rely
on matching raw PDF bytes with a brittle regex. Update the two tests in
PDFExporter.test.tsx to count pages using a PDF parser such as pdf-lib instead
of matching /Type /Page in the buffer, and keep the assertions in the existing
image-scaling and caption cases tied to the PDF generated by
PDFExporter.toReactPDFDocument.
In `@packages/xl-pdf-exporter/src/pdf/pdfExporter.tsx`:
- Around line 32-37: The PAGE_HEIGHT constant in pdfExporter.tsx is manually
synchronized with the <Page size="A4"> prop and can drift silently. Update the
pdfExporter/toReactPDFDocument flow to use a single shared PAGE_SIZE source for
both the <Page> component and PAGE_HEIGHT, or add a runtime/test assertion that
verifies the computed height still matches the page size used by
toReactPDFDocument and the maxHeight clamp in blocks.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ebcee73-1b04-418d-80a7-e38259f43f4f
⛔ Files ignored due to path filters (3)
packages/xl-pdf-exporter/src/pdf/__snapshots__/example.jsxis excluded by!**/__snapshots__/**packages/xl-pdf-exporter/src/pdf/__snapshots__/exampleWithAlignedImages.jsxis excluded by!**/__snapshots__/**packages/xl-pdf-exporter/src/pdf/__snapshots__/exampleWithHeaderAndFooter.jsxis excluded by!**/__snapshots__/**
📒 Files selected for processing (4)
packages/xl-pdf-exporter/src/pdf/defaultSchema/blocks.tsxpackages/xl-pdf-exporter/src/pdf/pdfExporter.test.tsxpackages/xl-pdf-exporter/src/pdf/pdfExporter.tsxshared/util/testFileResolver.ts
1 participant
Disclaimer: AI code, tested by a human, PR by a human.
Summary
This PR modifies image handling in the pdf export so aspect ratios are preserved and no unnecessary whitespace is created.
Rationale
Currently, images get squished, lose their resolution, or otherwise can get malformed in a pdf export. See #2866
Changes
Impact
Exported PDF documents might look slightly different. I have not found any real problematic side effects yet in my testing.
Testing
Added automated tests. Also tested manually with images of different sizes and locations.
Screenshots/Video
after.pdf
before.pdf
Checklist
Additional Notes
Fixes #2866
Tests now patch the jpeg sizes. Not ideal but seems to work fine. AI code might not fully match your expected code style so if you wish to use the changes in a separate commit, feel free to.
Summary by CodeRabbit
New Features
Bug Fixes