🐛(frontend) export any raster image supported by the browser to a PDF#2530
Open
magopian wants to merge 2 commits into
Open
🐛(frontend) export any raster image supported by the browser to a PDF#2530magopian wants to merge 2 commits into
magopian wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdded raster Blob-to-PNG conversion for PDF export, including canvas resizing and dimension handling. Updated image block mapping to use converted PNG data and handle conversion failures. Added unit tests for conversion and PDF element output, plus an end-to-end regression test confirming PNG content in exported PDFs. Documented the fix in the unreleased changelog. Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🤖 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.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-export/__tests__/imagePDF.test.tsx`:
- Around line 53-66: Update findInTree to use an explicit React element props
type covering children and the src/style fields accessed by its callers, and
apply that type to the return value and relevant casts. Preserve the recursive
traversal while ensuring matched elements expose typed props instead of unknown.
In
`@src/frontend/apps/impress/src/features/docs/doc-export/blocks-mapping/imagePDF.tsx`:
- Around line 28-31: Update the image conversion flow around convertBlobToPng to
catch rejected conversion errors and return the existing empty-image fallback,
View wrap={false}, instead of aborting the PDF export. Preserve the current
handling for undefined results and assign pngConverted and dimensions only after
a successful conversion.
In `@src/frontend/apps/impress/src/features/docs/doc-export/utils.ts`:
- Around line 110-134: Update the image export flow around createImageBitmap and
the canvas operations to wrap getContext, drawImage, and toDataURL in
try/finally, ensuring bmp.close() executes on both success and every failure
path. Preserve the existing sizing and returned PNG behavior, and add a test
covering an exception before the normal return to verify the ImageBitmap is
closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a32c5765-abb8-4a96-b09f-8f74f5375317
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-export.spec.tssrc/frontend/apps/impress/src/features/docs/doc-export/__tests__/imagePDF.test.tsxsrc/frontend/apps/impress/src/features/docs/doc-export/__tests__/utils.test.tssrc/frontend/apps/impress/src/features/docs/doc-export/blocks-mapping/imagePDF.tsxsrc/frontend/apps/impress/src/features/docs/doc-export/utils.ts
magopian
force-pushed
the
fix/image-export-to-pdf
branch
from
July 15, 2026 15:52
6613e76 to
b61e00e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-export/__tests__/imagePDF.test.tsx`:
- Around line 144-147: Update the assertions for the `pdfImage` result in
`imagePDF.test.tsx` to safely access the optional `props.style` before checking
width and height. Use a type-narrowing guard or optional chaining while
preserving the expected values of 450 and 225.
In
`@src/frontend/apps/impress/src/features/docs/doc-export/blocks-mapping/imagePDF.tsx`:
- Around line 34-40: Clamp previewWidth to MAX_WIDTH before invoking
convertSvgToPng or convertBlobToPng in the image conversion try block. Use the
clamped value for canvas allocation while preserving the existing
post-conversion result handling and fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58f20b77-fd84-4975-9f57-a1b8385c4659
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-export.spec.tssrc/frontend/apps/impress/src/features/docs/doc-export/__tests__/imagePDF.test.tsxsrc/frontend/apps/impress/src/features/docs/doc-export/__tests__/utils.test.tssrc/frontend/apps/impress/src/features/docs/doc-export/blocks-mapping/imagePDF.tsxsrc/frontend/apps/impress/src/features/docs/doc-export/utils.ts
Comment on lines
+144
to
+147
| const imageEl = findInTree(result as React.ReactNode, 'pdfImage'); | ||
| expect(imageEl).toBeDefined(); | ||
| expect(imageEl!.props.style.width).toBe(450); | ||
| expect(imageEl!.props.style.height).toBe(225); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'tsconfig*.json' src/frontend/apps/impress src/frontend/apps \
-x rg -n -C2 '"strict"|"strictNullChecks"' {}Repository: suitenumerique/docs
Length of output: 535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test and the PDF element type definition around the reported lines.
sed -n '1,280p' src/frontend/apps/impress/src/features/docs/doc-export/__tests__/imagePDF.test.tsx
printf '\n----\n'
rg -n "interface PDFElementProps|type PDFElementProps|style\\??:" src/frontend/apps/impress/src/features/docs -S
printf '\n----\n'
# Show surrounding lines for the type definition if found.
TYPE_FILE=$(rg -l "PDFElementProps" src/frontend/apps/impress/src/features/docs -S | head -n 1)
if [ -n "${TYPE_FILE:-}" ]; then
echo "TYPE_FILE=$TYPE_FILE"
rg -n -C3 "PDFElementProps|style\\??:" "$TYPE_FILE"
fiRepository: suitenumerique/docs
Length of output: 12657
Guard the optional style prop in these assertions. PDFElementProps.style is optional, so imageEl!.props.style.width/height fail under the repo’s strict TypeScript config. Narrow style first or use optional chaining in these assertions.
🤖 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
`@src/frontend/apps/impress/src/features/docs/doc-export/__tests__/imagePDF.test.tsx`
around lines 144 - 147, Update the assertions for the `pdfImage` result in
`imagePDF.test.tsx` to safely access the optional `props.style` before checking
width and height. Use a type-narrowing guard or optional chaining while
preserving the expected values of 450 and 225.
WebP format isn't supported by react-pdf/renderer and so wasn't exported properly, and some PNG images were also not exporting. First drawing those raster images to a canvas and providing a dataURL to react-pdf/renderer fixes those two bugs at once. Signed-off-by: Mathieu Agopian <mathieu@agopian.info>
Signed-off-by: Mathieu Agopian <mathieu@agopian.info>
magopian
force-pushed
the
fix/image-export-to-pdf
branch
from
July 15, 2026 16:10
b61e00e to
7c2d121
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #860
Purpose
WebP format isn't supported by react-pdf/renderer and so wasn't exported properly, and some PNG images were also not exporting.
Proposal
First drawing those raster images to a canvas and providing a dataURL to react-pdf/renderer fixes those two bugs at once.
External contributions
Thank you for your contribution! 🎉
Please ensure the following items are checked before submitting your pull request:
General requirements
Skip the checkbox below 👇 if you're fixing an issue or adding documentation
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)AI requirements
Skip the checkboxes below 👇 If you didn't use AI for your contribution