Import PPTX shape-level blipFill as ImageElement#284
Conversation
Modern PPTX templates routinely build full-bleed slide backgrounds and decorations as `<p:sp>` wrapping `<a:custGeom>` (or `<a:prstGeom rect>`) + `<a:blipFill>`, which is semantically equivalent to `<p:pic>`. The shape parser used to drop every shape-level blipFill silently (the existing `parseShapeFill` only handled `<a:solidFill>`), so imported doodle / pastel template decks landed as blank rectangles wherever the artwork should be — the user-reported "background not showing" case on the Multicolor Pastel Doodle deck. `parseSp` now detects `<a:blipFill>` in `<p:spPr>` before the `prstGeom` branch and emits an `ImageElement` via the existing `parseBlipFill` helper (shared with `<p:pic>` parsing — same uploader, crop, opacity handling). If the same shape also carries visible text, a coincident `TextElement` is layered on top, matching the existing `prstGeom + txBody` dual-emission pattern. When the blip fails to upload, we fall through so the shape still contributes whatever geometry or text it has rather than vanishing. Genuinely non-rect freeforms (e.g. an ellipse-clipped photo) lose their clip path under this approach — accepted as a v1 tradeoff because rectangular freeforms dominate real-world exports and the alternative is an empty rectangle. Gradient / pattern fills and master / layout `<p:bgRef>` resolution remain unhandled and are tracked separately as PPTX import gaps.
The previous comment in `parseShapeFill` claimed blip-fill on a shape was out of v1 scope. The preceding commit moved blip-fill handling into `parseSp` (it now short-circuits to an `ImageElement` before reaching `parseShapeFill`), so the comment misled the next reader into thinking the case was still unhandled. Tighten the wording to list only gradient/pattern as out of scope and point at the new branch for blip fills.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 48 minutes and 24 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements parsing of ChangesPPTX Shape Blip Fill Import Support
Sequence DiagramsequenceDiagram
participant parseSpTree
participant parseChild
participant parseSp
participant buildImageFromBlip
participant parseBlipFill
participant uploadImage
parseSpTree->>parseChild: iterate shape elements
parseChild->>parseSp: await parseSp(sp element)
parseSp->>parseSp: detect spPr/blipFill?
alt blipFill exists
parseSp->>buildImageFromBlip: build image from blip
buildImageFromBlip->>parseBlipFill: parse blip fill
parseBlipFill->>uploadImage: upload image bytes
uploadImage-->>buildImageFromBlip: image URL
buildImageFromBlip-->>parseSp: ImageElement with frame
parseSp->>parseSp: visible text present?
alt text exists
parseSp-->>parseChild: [ImageElement, TextElement]
else no text
parseSp-->>parseChild: [ImageElement]
end
else no blipFill
parseSp->>parseSp: parse prstGeom/custGeom
parseSp-->>parseChild: geometry-based elements
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (1)
packages/slides/test/import/pptx/shape-blipfill.test.ts (1)
182-193: ⚡ Quick winAdd a fallback test for
prstGeom + blipFillupload-failure behavior.Line 182 currently verifies only the
custGeomdrop case. Please also assert that whenblipFillfails on aprstGeomshape, parsing falls through and still emits the underlyingshape(not an empty result).Suggested test addition
+ it('falls through to shape parsing for prstGeom when blip upload fails', async () => { + const tree = spTreeFrom(SP_RECT_BLIPFILL); + const c = ctx({ rels: rels('../media/image1.png') }); // no uploadImage + + const out = await parseSpTree(tree, c); + + expect(out).toHaveLength(1); + expect(out[0].type).toBe('shape'); + expect(c.report.skippedImages).toBe(1); + });As per coding guidelines,
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases.🤖 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/slides/test/import/pptx/shape-blipfill.test.ts` around lines 182 - 193, Add a new test alongside the existing "falls through to existing path when blip upload fails" that constructs a sp tree representing a prstGeom shape with blipFill (e.g. the constant used for prst-geom+blip tests such as SP_PRST_GEOM_BLIPFILL), uses the same ctx({ rels: rels('../media/image1.jpeg') }) setup so uploadImage is missing, calls parseSpTree(tree, c) and then asserts that the parser emits the underlying shape (expect(out).toHaveLength(1) or expect(out.length).toBeGreaterThan(0)) instead of returning an empty array, and still increments c.report.skippedImages accordingly (expect(c.report.skippedImages).toBe(1)); use the same helpers spTreeFrom, ctx, rels and parseSpTree to place the test next to the existing one.
🤖 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/slides/test/import/pptx/shape-blipfill.test.ts`:
- Around line 182-193: Add a new test alongside the existing "falls through to
existing path when blip upload fails" that constructs a sp tree representing a
prstGeom shape with blipFill (e.g. the constant used for prst-geom+blip tests
such as SP_PRST_GEOM_BLIPFILL), uses the same ctx({ rels:
rels('../media/image1.jpeg') }) setup so uploadImage is missing, calls
parseSpTree(tree, c) and then asserts that the parser emits the underlying shape
(expect(out).toHaveLength(1) or expect(out.length).toBeGreaterThan(0)) instead
of returning an empty array, and still increments c.report.skippedImages
accordingly (expect(c.report.skippedImages).toBe(1)); use the same helpers
spTreeFrom, ctx, rels and parseSpTree to place the test next to the existing
one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af91a078-4594-4874-8196-dfc70f80dffd
📒 Files selected for processing (3)
docs/tasks/active/20260523-pptx-shape-blipfill-todo.mdpackages/slides/src/import/pptx/shape.tspackages/slides/test/import/pptx/shape-blipfill.test.ts
Verification: verify:selfResult: ✅ PASS in 200.4s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The existing fall-through test only exercised the custGeom path (empty output when no geometry to fall back to). Add a sibling test for the prstGeom + blipFill case, where the new branch must yield to the existing prstGeom branch so the underlying rectangle still renders. This locks in the graceful-degradation contract for image upload outages — without it a transient uploadImage failure could silently drop every picture-frame shape from the slide. Addresses CodeRabbit review feedback on PR #284.
|
@coderabbitai Addressed in 9a47343 — added a |
|
Tip For best results, initiate chat on the files or code changes.
(๑˃ᴗ˂)ﻌ ʕ•ᴥ•ʔ 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
These tasks' features merged to main (PRs #267 #271 #277 #284 #285) but their plan checkboxes were never ticked, so the todo files lingered in tasks/active. Added a status note citing each merge commit, marked the checkboxes complete, and ran tasks:archive to move them under archive/2026/05 with regenerated indexes. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
1 participant
Summary
<p:sp>wrapping<a:custGeom>(or<a:prstGeom rect>) +<a:blipFill>, which is semantically equivalent to<p:pic>. The shape parser previously dropped every shape-level blip fill (parseShapeFillonly handled<a:solidFill>), so imported decks landed as blank rectangles wherever the artwork should have been.parseSpnow detects<a:blipFill>in<p:spPr>before theprstGeombranch and emits anImageElementvia the existingparseBlipFillhelper (same uploader / crop / opacity path as<p:pic>). If the same shape also carries visible text, a coincidentTextElementis layered on top, matching the existingprstGeom + txBodydual-emission pattern.<p:bgRef>resolution remain unhandled and are tracked separately.Test plan
pnpm verify:fast— all 349 test files pass (sheets + docs + slides + frontend + backend)custGeom+ blipFill,prstGeom rect+ blipFill, blipFill + text overlay, upload-failure fallback, solid-fill control/slides/import, confirm slide-1 cream background + doodles renderTask doc
docs/tasks/active/20260523-pptx-shape-blipfill-todo.md🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests