add pptx export suppport to workspace-slides blueprint - #434
add pptx export suppport to workspace-slides blueprint#434maxwellpeterson wants to merge 15 commits into
Conversation
Preview:
|
|
@maxwellpeterson Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 2 actionable inline findings. |
…atim - Replace the slides ZIP writer with the workspace-sheets copy and adapt deckToPptx to its string/stream entry contract; batch slide XML into 64 KiB chunks like xlsx.js. - Parse highlight terms incrementally (max 128 comma-delimited entries), bound search work and highlight run transitions per title and per deck before any stream is returned, and stream runs directly from a shared mark array instead of materializing segments. - Drop the duplicated ZIP32 and determinism tests; add regressions for the reported attacks, boundaries, cross-line highlights, and bundled blueprint dependency wiring.
171a97c to
07127a1
Compare
|
APIError: You do not have access to the organization tied to the API key. |
2 similar comments
|
APIError: You do not have access to the organization tied to the API key. |
|
APIError: You do not have access to the organization tied to the API key. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Google Slides renders exported decks with loose paragraphs and wrapped logo/section labels. Emit CSS line-height as exact spcPts (spcPct is relative to Arial's ~115% line gap), size single-line boxes from Arial advance widths (Google ignores wrap="none"), keep the accent dot at the browser's 3px gap, and enlarge the bullet glyph to the browser's 6px dot.
|
APIError: You do not have access to the organization tied to the API key. |
2 similar comments
|
APIError: You do not have access to the organization tied to the API key. |
|
APIError: You do not have access to the organization tied to the API key. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
…etDeck schema stamp - estimateTextHeight wraps greedily at spaces with Arial metrics and the run's tracking; auto-height text shapes also use spAutoFit so a consumer's own wrapping can only grow the box, never clip. - The brand-bar shortcut requires the SVG to be built only from svg/defs/linearGradient/stop/rect; authored content keeps the placeholder. - Empty card/box titles occupy no height and emit no shape. - setDeck stamps themeVersion so a deck written over the GADGET binding is not discarded by the next getDeck (client boot or export).
|
Re Bonk's three findings (addressed in 2ca67ed):
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Google Slides converts spcPts back into a percentage of the font's natural line height, so exact points rendered ~15% loose there; a percentage divided by Arial's 1.15em natural height is what both PowerPoint and Google honour.
Targeted PPTX tests pass. |
…ce, card shrink-to-fit - normalizeXml and the run escaper copy clean spans instead of concatenating characters, so decks that alias one large block many times no longer build ropes the size of the deck text. - parseColor accepts rgb()/rgba() and the basic CSS named colors. - Props the browser renders with white-space normal/nowrap (section label, logo, pill, card eyebrow/title, box title/body, arrow label) collapse line breaks to spaces instead of becoming hard breaks. - Card text shapes emit normAutofit so consumers shrink text into the card, the nearest native equivalent of overflow: hidden.
|
Re Bonk's latest four (addressed in 9611593):
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
buSzPts is the bullet font size; Arial's black circle inks a 0.43em disc, so a 4.8pt (6px) marker needs an 11.16pt bullet font.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Posted 6 additional actionable inline findings. |
SVG markup becomes an image/svg+xml media part referenced through Office's svgBlip extension, byte for byte and deliberately unvalidated: the deck's SVG is trusted as authored (accepted risk; validation at the ingestion boundary is the follow-up). contain letterboxes the frame by the viewBox aspect; stretch fills the block. No raster fallback: consumers without SVG support show an empty frame. Media parts now carry their own extension/ mime/aspect so content types and contain math are shared with raster images.
24f66dc to
30a7f47
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
Consumers apply spcPct to the first line, so the line-height-1 wordmark's baseline moved and the dot, placed by a fixed offset, landed below it. Emit the wordmark at natural spacing with the box raised so its baseline lands where the browser's does, and put the dot 1px above that baseline after Chrome's trailing letter-spacing and the 3px gap.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Targeted PPTX tests pass: 27 tests. |
…hful layout - svgAspect parsed attributes with nested quantifiers (quadratic on a long malformed viewBox); attributes are now read with a linear scan and split. - prepareImageSource consults the source cache before scanning the value, so one large source referenced from thousands of blocks is read once; aggregate media budgets halved to fit the Worker heap alongside the deck. - Wrapped SVG pastes embed the <svg> element the browser renders; SVG files uploaded through the image control (data:image/svg+xml) are embedded too. - isBrandBar is structural: exactly the canonical elements and stop colors. - Layout parity: blocks without w shrink-to-fit up to the slide edge instead of fixed defaults; bullet items collapse inner whitespace; card eyebrows measure their height; overfull boxes centre their stack; pills stay intrinsic; omitted arrow endpoints are 0 like an absent SVG attribute; a letterboxed image is not rounded; dashes are a fixed 6px via custDash; the gadgets mark is a point-up hexagon with the scaled stroke; an empty wordmark keeps the dot's 3px gap.
|
Bonk's two rounds addressed in 4b1169e:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
svgElement sliced from the first opening tag to the last closing tag, so sibling elements produced a media part with several roots. A linear scan over svg open/close tags now tracks nesting (and quoted attributes) and returns exactly the first element, as the browser's querySelector does.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (!bytes) return {placeholder: "Malformed image data"}; | ||
| if (match[1] === "svg+xml") { | ||
| // The image control passes uploaded SVG files through verbatim (client.js fileToImageDataURL). | ||
| const element = svgElement(decoder.decode(bytes)); |
There was a problem hiding this comment.
[P1] Keep SVG upload conversion within the Worker heap. Two distinct valid 16 MiB SVG data URLs pass the 48 MiB encoded / 32 MiB decoded aggregate limits, but on the second image this path retains the deck’s ~43 MiB of base64, the first 16 MiB media buffer, and another decoded buffer, JS string, and re-encoded buffer. In a direct reproduction, deckToPptx() raised RSS from 101 MiB to 253 MiB before streaming, so a 128 MiB Worker can OOM despite all limits passing. Preserve the original decoded bytes while locating the SVG element, or apply a substantially lower SVG-specific budget.
| const bytes = decodeBase64(payload, decodedLength); | ||
| if (!bytes) return {placeholder: "Malformed image data"}; | ||
| if (match[1] === "svg+xml") { | ||
| // The image control passes uploaded SVG files through verbatim (client.js fileToImageDataURL). |
There was a problem hiding this comment.
[P2] Preserve valid non-UTF-8 SVG uploads. fileToImageDataURL() passes SVG files through verbatim, and XML permits encodings such as UTF-16, but the unconditional UTF-8 TextDecoder below turns a BOM-prefixed UTF-16 SVG into text with no recognizable <svg> and exports a Malformed image data placeholder. Detect the XML encoding or parse/extract directly from the original bytes.
| } | ||
|
|
||
| function svgMedia(element, mediaState) { | ||
| return findOrAddMedia(encoder.encode(element.source), { |
There was a problem hiding this comment.
[P2] Make pasted SVG fragments valid standalone SVG media. For <svg viewBox="0 0 10 10"><rect/></svg>, the editor’s HTML parser creates an SVG-namespaced node and renders it, but this byte-for-byte encoding leaves the media root without xmlns="http://www.w3.org/2000/svg". Chromium rejects that standalone image/svg+xml, and PowerPoint can consequently show a blank frame. Serialize the selected node or inject the SVG namespace when it is absent.
| // The value of `name="..."` in an element's opening tag, or null. A plain scan with no nested | ||
| // quantifiers: the tag is authored text and can be long, so parsing must stay linear. | ||
| function svgAttribute(rootTag, name) { | ||
| const match = new RegExp(String.raw`\s${name}\s*=\s*(?:"([^"]*)"|'([^']*)')`).exec(rootTag); |
There was a problem hiding this comment.
[P2] Skip quoted values while locating an SVG attribute. With valid markup <svg aria-label='chart viewBox = "0 0 200 100"' viewBox="0 0 100 100">…</svg>, this regex reads the attribute-like text inside aria-label, so a square SVG in a 400×400 contain block exports as 400×200 while the browser uses the real square viewBox. A linear opening-tag scanner should track quotes and match only actual attribute names.
| if (!color || width === 0) return "<a:ln><a:noFill/></a:ln>"; | ||
| let xml = `<a:ln w="${width}" cap="rnd">${solidFill(color, shapeOpacity)}`; | ||
| if (dashed) { | ||
| const dash = Math.round(6 / (width / PX_TO_LINE_EMU) * 100000); |
There was a problem hiding this comment.
[P2] Bound the custom-dash percentages before writing OOXML. A persisted dashed arrow with width: 0.0001 rounds to a 1-EMU line and emits d="6096000000" sp="6096000000"; CT_DashStop uses ST_PositivePercentage, whose integer maximum is 2147483647. This produces schema-invalid DrawingML that consumers may repair or drop. Clamp the percentage or enforce a representable minimum dashed width.
|
Posted 5 actionable inline findings. Targeted PPTX tests pass: 30/30. |
pptx.js no longer knows the `logo` block. server.js exports normalizeDeckForPptx(), a pure adapter that rewrites each logo into a text block (wordmark) and an ellipse shape (accent dot) at the logo's z-order position and previous geometry, using the renderer's new measureText() export for the wordmark's width and baseline. Text blocks honour letterSpacing in both renderers. Another slides blueprint can now copy pptx.js byte for byte and write its own adapter. Review findings: - svgElement steps over comments, CDATA and <!/<? declarations, skips every tag's quoted attribute values, ignores stray end tags. - tagAttribute is a linear attribute scanner, so attribute-like text in another value is never read (svgAttribute regex). - A pasted root without xmlns gets the SVG namespace declaration. - Uploaded SVG files are decoded by BOM / XML declaration (UTF-16, Latin-1, ...) and have a 4 MiB budget of their own. - isBrandBar compares tags as found and shares the per-source cache. - custDash percentages are clamped to the schema's integer maximum. - The gadgets mark's rotated box rounds to integer EMU. - Cards and boxes without w/h are sized to their content. - SVG aspect comes from viewBox only, as the browser overrides width/height with 100%. - Title highlights apply in term order, as the browser wraps them.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Google Slides, Quick Look and older PowerPoint render an svgBlip as an empty frame, and the exporter has no rasterizer for the PNG fallback such a picture needs, so SVG blocks and SVG uploads become the visible "SVG not included" placeholder again (the brand-bar gradient stays). The SVG scanner, encoding detection and svgBlip picture path go with it. Review findings: - normalizeDeckForPptx stops measuring wordmarks once they alone exceed the renderer's total-text limit (exported as MAX_TOTAL_TEXT_LENGTH), so an aliased huge logo cannot buy unbounded work ahead of validation. - A JPEG with a second SOF header is rejected instead of the later frame overwriting the dimensions the pixel limits were checked against. - Media are deduplicated by source string only; the byte comparison behind CRC32 buckets is gone. - Arrowheads use the largest preset (5x5), closest to the browser's 9x6 stroke-scaled marker. - viewProps grid spacing is PowerPoint's default 1/12 inch.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| let result = mediaState.bySource.get(markup); | ||
| if (!result) { | ||
| result = isBrandBar(markup) ? {brandBar: true} : {placeholder: "SVG not included"}; | ||
| mediaState.bySource.set(markup, result); |
There was a problem hiding this comment.
🟡 Cross-type source cache corrupts blocks
When one string serves as image data and SVG markup, bySource returns the first block type's incompatible result. The later block becomes a placeholder or makes ZIP streaming fail.
Learn more
Image sources and SVG markup share mediaState.bySource, but their cached result shapes differ. prepareImageSource expects {media} or an image placeholder, while prepareSvgSource expects {brandBar} or an SVG placeholder. An image-first collision gives the SVG renderer {media}, so it passes undefined into text generation and the returned ZIP stream errors during consumption. An SVG-first collision gives the image renderer an SVG placeholder and silently omits valid image data.
Example: Use one valid PNG data URL as an image block's props.src and as a later SVG block's props.markup. The image caches {media}. The SVG reads that entry, reaches its placeholder path with no placeholder, and ZIP streaming fails instead of producing the deck.
Recommended fix: Keep separate image and SVG caches, or use collision-free typed keys. Preserve image deduplication and SVG recognition caching within each type.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const BRAND_BAR_STOPS = ["#FF6633", "#F6821F", "#FBAD41"]; | ||
|
|
||
| function isBrandBar(markup) { | ||
| const tags = /<([A-Za-z][\w:-]*)([^>]*)>/g; |
There was a problem hiding this comment.
[P1] Replace this regex with a monotonic scanner. "<a".repeat(n) makes [^>]* scan the remaining suffix and fail once for every <a, so runtime is quadratic even though no tag matches. Through deckToPptx(), 20,000 repetitions took 0.63 s and 40,000 took 2.69 s here; the accepted one-million-character markup limit extrapolates beyond the export Worker deadline. Malformed SVG entered via addBlock() should become a placeholder without consuming minutes of CPU.
| return normalizeXml(text); | ||
| } | ||
|
|
||
| function sourceScalar(value) { |
There was a problem hiding this comment.
[P1] Parse and bound numeric scalar strings during preparation. These strings are not charged to MAX_TOTAL_TEXT_LENGTH, and renderers repeatedly call trim()/Number() on them through numberOr() and isBold(). One shared title with weight = " ".repeat(300_000) aliased across the allowed 10,000 blocks took 13.2 s to consume; a roughly 1.2 MB scalar exceeds the export deadline while the deck still passes every text/block limit. Store the parsed bounded number once rather than retaining and rescanning the source string.
| } | ||
| const match = IMAGE_DATA_URL.exec(value); | ||
| if (!match) { | ||
| boundedText(value, label, limits); |
There was a problem hiding this comment.
[P2] Recognize intentionally omitted SVG uploads before applying the generic text limit. The image control preserves SVG files verbatim, but a valid 740 KiB SVG becomes a 1,010,374-character base64 URL and this call rejects the entire deck as overlong instead of returning the documented SVG not included placeholder. Apply an explicit source/media bound, then short-circuit SVG data URLs before boundedText().
|
Posted 3 actionable inline findings. |
2 participants
update built-in slides blueprint to support pptx export