fix(core): preserve images in image PPTX export - #281
Conversation
|
@SegawaBeer is attempting to deploy a commit to the open-slide Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a ChangesImage-preserving PPTX export
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Export as exportSlideAsImagePptx
participant PrintReady as waitForImages
participant HtmlToImage as toCanvas
participant Composer as drawFrameImagesOntoCanvas
Export->>PrintReady: wait for images to be loaded/decoded
PrintReady-->>Export: resolved (or timed out)
Export->>HtmlToImage: capture frame into canvas
HtmlToImage-->>Export: canvas
Export->>Composer: composite image elements onto canvas
Composer->>Composer: compute placement, clip, opacity, border-radius
Composer-->>Export: updated canvas
Export->>Export: canvasToBlob (PNG) and assemble PPTX
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/app/lib/export-pptx.ts (1)
129-150: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftA single tainted image can still abort the whole export.
drawImage()won’t catch the origin-clean failure here; the rejection surfaces atcanvas.toBlob(), and that promise is awaited inside the main loop. Add a per-frame fallback or skip compositing before touching the shared canvas so one bad image doesn’t stop every remaining slide.🤖 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/core/src/app/lib/export-pptx.ts` around lines 129 - 150, The export loop in export-pptx.ts still lets one tainted frame abort the whole PPTX export because the failure is surfacing later at canvasToBlob() inside the shared for-loop. Update the frame capture flow around toCanvas, drawFrameImagesOntoCanvas, and canvasToBlob so each frame is handled independently with a fallback/skip path when compositing or blob creation fails, and continue processing the remaining frames instead of throwing for a single bad slide.
🧹 Nitpick comments (1)
packages/core/src/app/lib/export-pptx.ts (1)
168-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the new geometry/compositing helpers.
getImagePlacement,getObjectFitDestRect,getCoverSourceRect,resolveObjectPosition, and the clipping helpers are pure functions with meaningful edge cases (object-fit variants, position keyword swapping, overflow intersection) but ship without dedicated tests, unlikewaitForImages.🤖 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/core/src/app/lib/export-pptx.ts` around lines 168 - 560, Add unit tests for the new pure geometry/compositing helpers in export-pptx.ts, especially getImagePlacement, getObjectFitDestRect, getCoverSourceRect, resolveObjectPosition, and the overflow clipping helpers. Cover the key edge cases called out in the review: object-fit modes (contain, cover, scale-down, none, fill), keyword swapping for vertical object-position values, percentage/keyword resolution, and overflow intersection behavior in getCanvasOverflowClipRect/intersectRects. Keep the tests focused on these helper symbols so regressions in the export image rendering logic are caught without relying on the full export flow.
🤖 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 `@packages/core/src/app/lib/export-pptx.ts`:
- Around line 499-535: The border-radius clipping in applyBorderRadiusClip is
collapsing all corners to one uniform value via Math.max, which causes top-only
or asymmetric radii to export incorrectly. Update the clipping logic to respect
each corner’s individual radius from CSSStyleDeclaration (top-left, top-right,
bottom-right, bottom-left) instead of computing a single shared radius, and
build the clip path in export-pptx.ts accordingly so it matches the live DOM.
- Around line 168-227: The redraw pass in drawFrameImagesOntoCanvas is painting
every <img> after the base toCanvas capture, which can overpaint captions,
badges, and other later DOM layers. Update export-pptx so image redraw preserves
stacking order by only redrawing images when needed and in DOM/z-order relative
to overlapping content, using drawFrameImagesOntoCanvas and its
getImagePlacement/getCanvasBorderBox flow as the place to apply the ordering
fix.
---
Outside diff comments:
In `@packages/core/src/app/lib/export-pptx.ts`:
- Around line 129-150: The export loop in export-pptx.ts still lets one tainted
frame abort the whole PPTX export because the failure is surfacing later at
canvasToBlob() inside the shared for-loop. Update the frame capture flow around
toCanvas, drawFrameImagesOntoCanvas, and canvasToBlob so each frame is handled
independently with a fallback/skip path when compositing or blob creation fails,
and continue processing the remaining frames instead of throwing for a single
bad slide.
---
Nitpick comments:
In `@packages/core/src/app/lib/export-pptx.ts`:
- Around line 168-560: Add unit tests for the new pure geometry/compositing
helpers in export-pptx.ts, especially getImagePlacement, getObjectFitDestRect,
getCoverSourceRect, resolveObjectPosition, and the overflow clipping helpers.
Cover the key edge cases called out in the review: object-fit modes (contain,
cover, scale-down, none, fill), keyword swapping for vertical object-position
values, percentage/keyword resolution, and overflow intersection behavior in
getCanvasOverflowClipRect/intersectRects. Keep the tests focused on these helper
symbols so regressions in the export image rendering logic are caught without
relying on the full export flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3aadb4fa-c69c-4c7c-87e7-ec1846aa64ff
📒 Files selected for processing (4)
.changeset/few-clean-slides.mdpackages/core/src/app/lib/export-pptx.tspackages/core/src/app/lib/print-ready.test.tspackages/core/src/app/lib/print-ready.ts
| function drawFrameImagesOntoCanvas( | ||
| frame: HTMLElement, | ||
| canvas: HTMLCanvasElement, | ||
| fallbackPixelRatio: number, | ||
| ): void { | ||
| const ctx = canvas.getContext('2d'); | ||
| if (!ctx) return; | ||
|
|
||
| const frameRect = frame.getBoundingClientRect(); | ||
| if (frameRect.width <= 0 || frameRect.height <= 0) return; | ||
|
|
||
| const scaleX = canvas.width / frameRect.width || fallbackPixelRatio; | ||
| const scaleY = canvas.height / frameRect.height || fallbackPixelRatio; | ||
| const images = Array.from(frame.querySelectorAll<HTMLImageElement>('img')); | ||
|
|
||
| for (const img of images) { | ||
| const placement = getImagePlacement(frame, img, frameRect); | ||
| if (!placement) continue; | ||
|
|
||
| const computedStyle = getComputedStyle(img); | ||
| const clipRect = getCanvasBorderBox(frameRect, img.getBoundingClientRect(), scaleX, scaleY); | ||
| const overflowClipRect = getCanvasOverflowClipRect(frame, img, frameRect, scaleX, scaleY); | ||
|
|
||
| ctx.save(); | ||
| if (overflowClipRect) { | ||
| ctx.beginPath(); | ||
| ctx.rect( | ||
| overflowClipRect.x, | ||
| overflowClipRect.y, | ||
| overflowClipRect.width, | ||
| overflowClipRect.height, | ||
| ); | ||
| ctx.clip(); | ||
| } | ||
| applyBorderRadiusClip(ctx, clipRect, computedStyle); | ||
| ctx.globalAlpha = placement.opacity; | ||
| if (computedStyle.filter && computedStyle.filter !== 'none') { | ||
| ctx.filter = computedStyle.filter; | ||
| } | ||
|
|
||
| try { | ||
| ctx.drawImage( | ||
| img, | ||
| placement.source.x, | ||
| placement.source.y, | ||
| placement.source.width, | ||
| placement.source.height, | ||
| placement.dest.x * scaleX, | ||
| placement.dest.y * scaleY, | ||
| placement.dest.width * scaleX, | ||
| placement.dest.height * scaleY, | ||
| ); | ||
| } catch { | ||
| // Cross-origin images without CORS can taint or reject canvas drawing. | ||
| // Keep the base html-to-image capture instead of failing the whole export. | ||
| } finally { | ||
| ctx.restore(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== export-pptx relevant range ==\n'
sed -n '1,260p' packages/core/src/app/lib/export-pptx.ts
printf '\n== search for layering / z-index / overlays in app components ==\n'
rg -n --hidden --glob '!**/dist/**' --glob '!**/build/**' --glob '!**/node_modules/**' \
-e 'z-index|zIndex|overlay|watermark|caption|badge|absolute|position:\s*["'\'']?absolute|position:\s*["'\'']?relative' \
packages/core/src/app | sed -n '1,220p'
printf '\n== search for img usage in slide/layout components ==\n'
rg -n --hidden --glob '!**/dist/**' --glob '!**/build/**' --glob '!**/node_modules/**' \
-e '<img|Image|next/image|picture|background-image|object-fit' \
packages/core/src/app | sed -n '1,220p'Repository: 1weiho/open-slide
Length of output: 42318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== top-level core app files ==\n'
git ls-files packages/core/src/app | sed -n '1,260p'
printf '\n== likely slide/page modules outside app ==\n'
git ls-files packages/core/src | rg '^packages/core/src/(slides|slide|pages|content|deck|present|app/routes|app/components)/' | sed -n '1,260p'
printf '\n== search for SlideModule/default page definitions ==\n'
rg -n --hidden --glob '!**/dist/**' --glob '!**/build/**' --glob '!**/node_modules/**' \
-e 'export default|slide\.default|type SlideModule|interface SlideModule|define.*Slide|Page =' \
packages/core/src | sed -n '1,260p'Repository: 1weiho/open-slide
Length of output: 29742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== slide module files in repo ==\n'
git ls-files | rg '(^|/)(slides?|examples?|demo|deck|pages?)/.*\.(tsx?|jsx?)$|\.slide\.(tsx?|jsx?)$|\.page\.(tsx?|jsx?)$' | sed -n '1,260p'
printf '\n== img + overlay patterns in slide/module files ==\n'
git ls-files | rg '\.(tsx?|jsx?)$' | while read -r f; do
if rg -n -e '<img|backgroundImage|zIndex|z-index|absolute|overlay|caption|badge|watermark' "$f" >/dev/null 2>&1; then
printf '\n--- %s ---\n' "$f"
rg -n -e '<img|backgroundImage|zIndex|z-index|absolute|overlay|caption|badge|watermark' "$f" | sed -n '1,60p'
fi
doneRepository: 1weiho/open-slide
Length of output: 42258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== inspect image-placement helpers ==\n'
sed -n '229,560p' packages/core/src/app/lib/export-pptx.ts
printf '\n== inspect any export-pdf analog for drawing order ==\n'
sed -n '1,260p' packages/core/src/app/lib/export-pdf.tsRepository: 1weiho/open-slide
Length of output: 18441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== app/lib/slides.ts ==\n'
cat -n packages/core/src/app/lib/slides.ts
printf '\n== sdk types ==\n'
cat -n packages/core/src/app/lib/sdk.ts
printf '\n== slide route rendering relevant excerpt ==\n'
sed -n '100,220p' packages/core/src/app/routes/slide.tsxRepository: 1weiho/open-slide
Length of output: 6637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== theme presets and demo pages with images ==\n'
rg -n --hidden --glob '!**/dist/**' --glob '!**/build/**' --glob '!**/node_modules/**' \
-e '<img|backgroundImage|ImagePlaceholder|objectFit|objectPosition|overlay|zIndex|position:\s*["'\'']?absolute' \
packages/core/src/app/lib packages/core/src/app/components/themes packages/core/src/app/routes | sed -n '1,240p'Repository: 1weiho/open-slide
Length of output: 1820
Image redraw pass can cover overlays
drawFrameImagesOntoCanvas redraws every <img> after toCanvas(), so any caption, badge, or other layer stacked above an image in the DOM will be overpainted in the PPTX export. This should preserve stacking order instead of always painting images last.
🤖 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/core/src/app/lib/export-pptx.ts` around lines 168 - 227, The redraw
pass in drawFrameImagesOntoCanvas is painting every <img> after the base
toCanvas capture, which can overpaint captions, badges, and other later DOM
layers. Update export-pptx so image redraw preserves stacking order by only
redrawing images when needed and in DOM/z-order relative to overlapping content,
using drawFrameImagesOntoCanvas and its getImagePlacement/getCanvasBorderBox
flow as the place to apply the ordering fix.
| function applyBorderRadiusClip( | ||
| ctx: CanvasRenderingContext2D, | ||
| rect: Rect, | ||
| style: CSSStyleDeclaration, | ||
| ): void { | ||
| const radius = Math.max( | ||
| cssRadius(style.borderTopLeftRadius, rect.width), | ||
| cssRadius(style.borderTopRightRadius, rect.width), | ||
| cssRadius(style.borderBottomRightRadius, rect.width), | ||
| cssRadius(style.borderBottomLeftRadius, rect.width), | ||
| ); | ||
|
|
||
| if (radius <= 0) { | ||
| ctx.beginPath(); | ||
| ctx.rect(rect.x, rect.y, rect.width, rect.height); | ||
| ctx.clip(); | ||
| return; | ||
| } | ||
|
|
||
| const r = Math.min(radius, rect.width / 2, rect.height / 2); | ||
| ctx.beginPath(); | ||
| ctx.moveTo(rect.x + r, rect.y); | ||
| ctx.lineTo(rect.x + rect.width - r, rect.y); | ||
| ctx.quadraticCurveTo(rect.x + rect.width, rect.y, rect.x + rect.width, rect.y + r); | ||
| ctx.lineTo(rect.x + rect.width, rect.y + rect.height - r); | ||
| ctx.quadraticCurveTo( | ||
| rect.x + rect.width, | ||
| rect.y + rect.height, | ||
| rect.x + rect.width - r, | ||
| rect.y + rect.height, | ||
| ); | ||
| ctx.lineTo(rect.x + r, rect.y + rect.height); | ||
| ctx.quadraticCurveTo(rect.x, rect.y + rect.height, rect.x, rect.y + rect.height - r); | ||
| ctx.lineTo(rect.x, rect.y + r); | ||
| ctx.quadraticCurveTo(rect.x, rect.y, rect.x + r, rect.y); | ||
| ctx.clip(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Border-radius clip collapses all four corners into one uniform radius.
Math.max over all four corner radii (Lines 504-509) means an image styled with e.g. only rounded top corners (a common card-header pattern) will render with all four corners rounded in the export, diverging from the live DOM.
💡 Use each corner's own radius instead of a single max
function applyBorderRadiusClip(
ctx: CanvasRenderingContext2D,
rect: Rect,
style: CSSStyleDeclaration,
): void {
- const radius = Math.max(
- cssRadius(style.borderTopLeftRadius, rect.width),
- cssRadius(style.borderTopRightRadius, rect.width),
- cssRadius(style.borderBottomRightRadius, rect.width),
- cssRadius(style.borderBottomLeftRadius, rect.width),
- );
-
- if (radius <= 0) {
+ const maxR = Math.min(rect.width / 2, rect.height / 2);
+ const tl = Math.min(cssRadius(style.borderTopLeftRadius, rect.width), maxR);
+ const tr = Math.min(cssRadius(style.borderTopRightRadius, rect.width), maxR);
+ const br = Math.min(cssRadius(style.borderBottomRightRadius, rect.width), maxR);
+ const bl = Math.min(cssRadius(style.borderBottomLeftRadius, rect.width), maxR);
+
+ if (tl <= 0 && tr <= 0 && br <= 0 && bl <= 0) {
ctx.beginPath();
ctx.rect(rect.x, rect.y, rect.width, rect.height);
ctx.clip();
return;
}
-
- const r = Math.min(radius, rect.width / 2, rect.height / 2);
ctx.beginPath();
- ctx.moveTo(rect.x + r, rect.y);
- ctx.lineTo(rect.x + rect.width - r, rect.y);
- ctx.quadraticCurveTo(rect.x + rect.width, rect.y, rect.x + rect.width, rect.y + r);
- ctx.lineTo(rect.x + rect.width, rect.y + rect.height - r);
+ ctx.moveTo(rect.x + tl, rect.y);
+ ctx.lineTo(rect.x + rect.width - tr, rect.y);
+ ctx.quadraticCurveTo(rect.x + rect.width, rect.y, rect.x + rect.width, rect.y + tr);
+ ctx.lineTo(rect.x + rect.width, rect.y + rect.height - br);
ctx.quadraticCurveTo(
rect.x + rect.width,
rect.y + rect.height,
- rect.x + rect.width - r,
+ rect.x + rect.width - br,
rect.y + rect.height,
);
- ctx.lineTo(rect.x + r, rect.y + rect.height);
- ctx.quadraticCurveTo(rect.x, rect.y + rect.height, rect.x, rect.y + rect.height - r);
- ctx.lineTo(rect.x, rect.y + r);
- ctx.quadraticCurveTo(rect.x, rect.y, rect.x + r, rect.y);
+ ctx.lineTo(rect.x + bl, rect.y + rect.height);
+ ctx.quadraticCurveTo(rect.x, rect.y + rect.height, rect.x, rect.y + rect.height - bl);
+ ctx.lineTo(rect.x, rect.y + tl);
+ ctx.quadraticCurveTo(rect.x, rect.y, rect.x + tl, rect.y);
ctx.clip();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function applyBorderRadiusClip( | |
| ctx: CanvasRenderingContext2D, | |
| rect: Rect, | |
| style: CSSStyleDeclaration, | |
| ): void { | |
| const radius = Math.max( | |
| cssRadius(style.borderTopLeftRadius, rect.width), | |
| cssRadius(style.borderTopRightRadius, rect.width), | |
| cssRadius(style.borderBottomRightRadius, rect.width), | |
| cssRadius(style.borderBottomLeftRadius, rect.width), | |
| ); | |
| if (radius <= 0) { | |
| ctx.beginPath(); | |
| ctx.rect(rect.x, rect.y, rect.width, rect.height); | |
| ctx.clip(); | |
| return; | |
| } | |
| const r = Math.min(radius, rect.width / 2, rect.height / 2); | |
| ctx.beginPath(); | |
| ctx.moveTo(rect.x + r, rect.y); | |
| ctx.lineTo(rect.x + rect.width - r, rect.y); | |
| ctx.quadraticCurveTo(rect.x + rect.width, rect.y, rect.x + rect.width, rect.y + r); | |
| ctx.lineTo(rect.x + rect.width, rect.y + rect.height - r); | |
| ctx.quadraticCurveTo( | |
| rect.x + rect.width, | |
| rect.y + rect.height, | |
| rect.x + rect.width - r, | |
| rect.y + rect.height, | |
| ); | |
| ctx.lineTo(rect.x + r, rect.y + rect.height); | |
| ctx.quadraticCurveTo(rect.x, rect.y + rect.height, rect.x, rect.y + rect.height - r); | |
| ctx.lineTo(rect.x, rect.y + r); | |
| ctx.quadraticCurveTo(rect.x, rect.y, rect.x + r, rect.y); | |
| ctx.clip(); | |
| } | |
| function applyBorderRadiusClip( | |
| ctx: CanvasRenderingContext2D, | |
| rect: Rect, | |
| style: CSSStyleDeclaration, | |
| ): void { | |
| const maxR = Math.min(rect.width / 2, rect.height / 2); | |
| const tl = Math.min(cssRadius(style.borderTopLeftRadius, rect.width), maxR); | |
| const tr = Math.min(cssRadius(style.borderTopRightRadius, rect.width), maxR); | |
| const br = Math.min(cssRadius(style.borderBottomRightRadius, rect.width), maxR); | |
| const bl = Math.min(cssRadius(style.borderBottomLeftRadius, rect.width), maxR); | |
| if (tl <= 0 && tr <= 0 && br <= 0 && bl <= 0) { | |
| ctx.beginPath(); | |
| ctx.rect(rect.x, rect.y, rect.width, rect.height); | |
| ctx.clip(); | |
| return; | |
| } | |
| ctx.beginPath(); | |
| ctx.moveTo(rect.x + tl, rect.y); | |
| ctx.lineTo(rect.x + rect.width - tr, rect.y); | |
| ctx.quadraticCurveTo(rect.x + rect.width, rect.y, rect.x + rect.width, rect.y + tr); | |
| ctx.lineTo(rect.x + rect.width, rect.y + rect.height - br); | |
| ctx.quadraticCurveTo( | |
| rect.x + rect.width, | |
| rect.y + rect.height, | |
| rect.x + rect.width - br, | |
| rect.y + rect.height, | |
| ); | |
| ctx.lineTo(rect.x + bl, rect.y + rect.height); | |
| ctx.quadraticCurveTo(rect.x, rect.y + rect.height, rect.x, rect.y + rect.height - bl); | |
| ctx.lineTo(rect.x, rect.y + tl); | |
| ctx.quadraticCurveTo(rect.x, rect.y, rect.x + tl, rect.y); | |
| ctx.clip(); | |
| } |
🤖 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/core/src/app/lib/export-pptx.ts` around lines 499 - 535, The
border-radius clipping in applyBorderRadiusClip is collapsing all corners to one
uniform value via Math.max, which causes top-only or asymmetric radii to export
incorrectly. Update the clipping logic to respect each corner’s individual
radius from CSSStyleDeclaration (top-left, top-right, bottom-right, bottom-left)
instead of computing a single shared radius, and build the clip path in
export-pptx.ts accordingly so it matches the live DOM.
Summary
<img>elements from the live DOM so html-to-image omissions do not leave blank areasobject-fit,object-position, opacity, filters, border radius, and ancestor overflow clippingWhy
Export as image PPTXcould produce slides where large screenshot/image areas were blank even though the browser preview rendered correctly. This fixes that path by making sure loaded image pixels are present in the final per-slide PNGs written into the PPTX.Verification
pnpm exec biome check packages/core/src/app/lib/export-pptx.ts packages/core/src/app/lib/print-ready.ts packages/core/src/app/lib/print-ready.test.ts .changeset/few-clean-slides.mdpnpm test packages/core/src/app/lib/print-ready.test.tspnpm typecheck --filter=@open-slide/corepnpm --filter @open-slide/core buildpnpm --filter demo buildI also verified the fixed export path on a local deck with many screenshot assets: the generated image PPTX contained all slide images instead of blank screenshot regions.
Summary by CodeRabbit
New Features
Bug Fixes