issue-209: Implement the SVG-to-shapes converter in src/svg.ts with unit tests #242

Merged

david merged 2 commits from feature/issue-209/implement-the-svg-to-shapes-converter-in-src-svg-t into main

2026-09-15 06:12:30 +00:00

david commented

2026-09-15 06:12:29 +00:00

Owner

209: Implement the SVG-to-shapes converter in src/svg.ts with unit tests

Closes #209 — 209.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 22m 56s.

Review findings

Now let me make the edits to svg.ts. Starting with the module docs and interfaces:Now the xml.ts changes for mixed content and code point validation:I've completed the remediation pass. All findings were addressed; the only failing test in the full suite is the pre-existing, unrelated local-install.test.mjs manifest check the review already identified.

Review Findings — 209

Summary

All 13 findings were fixed in this pass. The converter's own gate (node --test extensions/penpot/src/svg.test.ts extensions/penpot/src/xml.test.ts) is now 98/98 green; the full npm test suite is 784 pass / 1 fail / 1 skipped, where the single failure (local-install.test.mjs, "every pi.extensions entry resolves to a file and mongodb is declared") reproduces on HEAD~1 with this diff absent and is pre-existing/unrelated. No build or lint gate exists in this repo, so build/lint remain N/A; no TS type-checker is installed either. Changes: <defs> is preserved as a single svg-raw instead of traversed (definitions no longer render as shapes); non-rendering state (display:none, visibility:hidden, opacity) is carried on ShapeDescriptor as hidden/opacity (opacity not inherited, to avoid multiplying a group's opacity); geometry/font-size lengths now resolve % against the root viewBox and reject other units with a reason, and parseNumber no longer drops suffixes; malformed transform argument lists are rejected (the review's suggested empty-field split was insufficient for matrix(1,,0,0,1,5 6), so an explicit comma validator was added); font-weight="bold" maps to 700; <use>/image fallbacks get real bounds and a zeroGeometry flag marks placeholders; the XML reader keeps mixed-content order via XmlNode.content and rejects illegal character references; the module/CHANGELOG/findings docs record the subset, the Penpot-2.17.2 comparisons and the follow-up tool step.

Critical

extensions/penpot/src/svg.ts:1448 — <defs> is traversed "transparently", so a definition is emitted as a first-class shape: convertSvg('<svg><defs><path id="a" d="M0 0 L10 10 Z"/></defs><use href="#a"/></svg>') returns a native path and a zero-size svg-raw, i.e. artwork that SVG never renders is drawn at the def's own coordinates while the <use> that does render is a 0×0 fallback. This is the classic icon shape (<defs><path/></defs><use/>) and it silently violates the documented "no node is dropped silently … rather than silent mis-conversion" contract (the doc's justification, "defs … carries definitions, not artwork", argues for the opposite of what the code does). Penpot's own importer does not do this: csvg/extract-defs pulls defs out of the content tree into the root shape's :svg-defs (/tmp/penpot-src/common/src/app/common/files/shapes_builder.cljc:216,272), never as shapes. Suggested fix: stop recursing into defs with the normal visiting context — either record the whole <defs> subtree as a single svg-raw skip (so the markup is preserved without being rendered), or drop defs from TRANSPARENT_ELEMENTS and give it its own unsupported element "defs" reason; then update the test at svg.test.ts ("convertSvg traverses defs transparently and reports its contents") to assert the defs content produces no native shape.

High

extensions/penpot/src/svg.ts:1474 — non-rendering state is ignored: <rect display="none">, visibility="hidden" and opacity="0" all produce an ordinary, fully visible shape (verified), because visit only guards UNSUPPORTED_ATTRIBUTES = ["mask","filter","clip-path"]. display:none means "do not render this element (nor its children)", so a hidden layer in an exported SVG reappears as visible artwork — the same silent-mis-conversion class as the defs bug, and undocumented in the module's "Known limitations". Penpot's importer maps it explicitly ((= (dm/get-in shape [:svg-attrs :display]) "none") … (assoc :hidden true), shapes_builder.cljc:640-650), and Penpot's shape schema has :hidden and :opacity. Suggested fix: read display/visibility/opacity (attribute and inline style) in mergeStyle; when display:none (or visibility:hidden, opacity:0) either carry a hidden/opacity field on ShapeDescriptor for the tool step (Penpot has both) or route the node to svg-raw with a specific reason — and add a test for each.
extensions/penpot/src/svg.ts:1115 — parseNumber uses Number.parseFloat, which silently ignores a unit suffix, so unit-bearing lengths are mis-scaled instead of falling back: <rect width="50%" height="50%"/> becomes a 50×50 rect, x="10pt" → 10, width="1cm" → 1, font-size="12pt" → 12 (all verified, no skipped entry). Only rootMatrix's parseAbsoluteLength is unit-aware, and it special-cases % only. This is silently wrong output for a documented-subset input, and the reference implementation resolves it rather than ignoring it (Penpot's csvg/fix-percents, /tmp/penpot-src/common/src/app/common/svg.cljc:950, scales % against the viewBox). Suggested fix: parse length values with an explicit unit check (""/px accepted; % resolved against the viewBox/root matrix; every other unit — em ex cm mm pt pc — a fallback with a reason such as unsupported length unit "pt"), and test a %-sized rect and a pt font-size.
extensions/penpot/src/svg.test.ts:1 — several implemented branches of the new converter have no test, including the only occurrences of two distinct fallback reasons: applyMatrixToRect's rounded-rect path (rx/ry → geometry.radius) and both REASON_ELLIPTICAL_RADIUS refusals (svg.ts:804-820); preserveAspectRatio="none" (svg.ts:1298, a documented feature); stroke-linecap="square" and the omitted-butt case (svg.ts:1244); geometryIsConsistent returning false for any kind (svg.ts:1000); and every unit-suffixed length (see the High finding above). Suggested fix: add focused tests — a rx="2" rect asserting geometry.radius === 2, a rx="2" ry="4" rect and a scale(2 1) rounded rect asserting the elliptical-radius fallback, a non-square viewBox with preserveAspectRatio="none", stroke-linecap="square" plus a stroke-linecap="butt" stroke with no strokeCapStart key, a hand-built inconsistent geometry asserting false, and a %/pt geometry fixture.

Medium

extensions/penpot/src/svg.ts:352 — argsText.split(/[\s,]+/).map(Number) turns empty argument fields into 0, so malformed transform syntax is accepted instead of failing as the module docs promise ("Malformed syntax is an explicit failure"): matrixFrom("scale(,)") → {a:0,d:0} (a silently collapsed zero-scale shape), matrixFrom("translate(,10)") → {e:0,f:10}, matrixFrom("matrix(1,,0,0,1,5 6)") → accepted (all verified). Suggested fix: split first and reject any empty field (if (parts.some(p => p.trim() === "")) return invalidTransform(value)), then Number() the parts; add tests for the three cases above.
extensions/penpot/src/svg.ts:1652 — parseNumber(node.attributes["font-weight"], 400) silently returns the 400 fallback for the keyword forms, so the very common font-weight="bold" is emitted as fontWeight: 400 with no fallback and no report entry (verified). Suggested fix: map normal|bold|bolder|lighter (and font-style if it is ever used) before falling back, or push the node to svg-raw with unsupported font-weight "bold"; add a test.
extensions/penpot/src/svg.ts:1712 — pushSkip derives svg-raw geometry from localBounds, which returns zeroBounds() for use, image, mask, a nested svg and anything unrecognised, so the most common fallback (present in the tests, e.g. <use href="#x"/>) is emitted as a 0×0 box — preserved markup the design will not show, and SkippedNode gives the caller no way to tell "preserved but invisible" from "preserved with real bounds". Suggested fix: resolve <use href="#id"> against the document's ids (and image against its width/height) for the fallback bounds, and/or add a zeroGeometry: true flag to SkippedNode (and a distinct reason) so the tool step can surface it.
extensions/penpot/src/xml.ts:390 — mixed content loses its ordering, because XmlNode.text accumulates all direct text while children are kept separately and serializeXml re-emits text first: <text x="0" y="10">a<tspan>b</tspan>c</text> is preserved as <text x="0" y="10">ac<tspan>b</tspan></text> (verified), i.e. the svg-raw fallback's markup — its whole reason for existing — is wrong. Suggested fix: represent text as positioned nodes (content: Array<string | XmlNode>) and serialise in order, or, if that is out of scope, document the limitation and make serializeXml reject/flag mixed content rather than silently reorder it; add a test.
extensions/penpot/src/svg.ts:44 — the module doc states "The reader is bounded by {@link DEFAULT_MAX_NODES} visited elements", but parseXml has no element-count bound (only MAX_XML_DEPTH), and the node budget is enforced during convertSvg's traversal after the whole document is parsed and materialised — so a hostile 10 M-element SVG is fully parsed into memory before SvgError is thrown (verified: 20 000 rects throw, but only after parsing). Suggested fix: correct the doc to say the traversal is bounded and the reader is bounded by depth only, or move the count into parseXml via an optional maxNodes so the bound holds before materialisation.
CHANGELOG.md:7 — issue #209 records nothing: the commit touches only the five source/test files, with no ## [Unreleased] entry and no extensions/penpot/findings.md section, while every prior issue in this extension did both — including the tool-less pure module of issue #202 ("Binfile Transit decoder + SSE parser — issue #202") and the source-verified pass of #208. Suggested fix: add a CHANGELOG ### Added entry describing the converter and XML reader (scope, documented subset, svg-raw fallback, 79 tests) and a ## SVG → native shapes converter — issue #209 section in extensions/penpot/findings.md recording the subset decisions and the two Penpot-2.17.2 comparisons above (defs extraction, display:none → hidden).

Low

extensions/penpot/src/svg.ts:210 — the svg-raw descriptor carries raw as a serialised markup string, but Penpot's svg-raw shape does not accept one: create-raw-svg stores content as hiccup data plus svg-attrs and svg-viewbox (/tmp/penpot-src/common/src/app/common/files/shapes_builder.cljc:270-284). The "ready for the tool-wrapping step" claim therefore cannot be honoured by passing raw through. Suggested fix: document on ShapeDescriptor.raw (and in the module doc) that the tool step must re-parse it into Penpot's content/svg-attrs/svg-viewbox, or emit that structure here.
extensions/penpot/src/xml.ts:321 — codePointToString accepts any code point in [0, 0x10ffff], so  yields "\0" and &#xD800; yields a lone surrogate (verified) — both illegal in XML and both able to reach preserved svg-raw markup. Suggested fix: also reject the C0 controls except tab/newline/carriage-return, 0xFFFE/0xFFFF, and the surrogate range 0xD800–0xDFFF.
extensions/penpot/src/index.ts:1 — the converter is not imported anywhere in the extension (only svg.test.ts references it), so nothing yet surfaces the report/skipped accounting to the agent, which is exactly what plan M6 asks the tool step to do ("Record what was converted vs. fell back, and surface that to the agent"). Suggested fix: if a follow-up issue owns penpot_add_svg, say so in the CHANGELOG/findings entry (see the Medium finding); otherwise the converter is unreachable code and that milestone remains open.

## 209: Implement the SVG-to-shapes converter in src/svg.ts with unit tests Closes #209 — [209](https://git.excelera.net/david/pi-extensions-and-skills/issues/209). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 22m 56s. ## Review findings Now let me make the edits to `svg.ts`. Starting with the module docs and interfaces:Now the `xml.ts` changes for mixed content and code point validation:I've completed the remediation pass. All findings were addressed; the only failing test in the full suite is the pre-existing, unrelated `local-install.test.mjs` manifest check the review already identified. # Review Findings — 209 ## Summary All 13 findings were fixed in this pass. The converter's own gate (`node --test extensions/penpot/src/svg.test.ts extensions/penpot/src/xml.test.ts`) is now **98/98 green**; the full `npm test` suite is **784 pass / 1 fail / 1 skipped**, where the single failure (`local-install.test.mjs`, "every pi.extensions entry resolves to a file and mongodb is declared") reproduces on `HEAD~1` with this diff absent and is pre-existing/unrelated. No build or lint gate exists in this repo, so build/lint remain N/A; no TS type-checker is installed either. Changes: `<defs>` is preserved as a single `svg-raw` instead of traversed (definitions no longer render as shapes); non-rendering state (`display:none`, `visibility:hidden`, `opacity`) is carried on `ShapeDescriptor` as `hidden`/`opacity` (opacity not inherited, to avoid multiplying a group's opacity); geometry/`font-size` lengths now resolve `%` against the root `viewBox` and reject other units with a reason, and `parseNumber` no longer drops suffixes; malformed transform argument lists are rejected (the review's suggested empty-field split was insufficient for `matrix(1,,0,0,1,5 6)`, so an explicit comma validator was added); `font-weight="bold"` maps to 700; `<use>`/`image` fallbacks get real bounds and a `zeroGeometry` flag marks placeholders; the XML reader keeps mixed-content order via `XmlNode.content` and rejects illegal character references; the module/CHANGELOG/findings docs record the subset, the Penpot-2.17.2 comparisons and the follow-up tool step. ## Critical - [x] `extensions/penpot/src/svg.ts:1448` — `<defs>` is traversed "transparently", so a definition is emitted as a first-class shape: `convertSvg('<svg><defs><path id="a" d="M0 0 L10 10 Z"/></defs><use href="#a"/></svg>')` returns a native `path` **and** a zero-size `svg-raw`, i.e. artwork that SVG never renders is drawn at the def's own coordinates while the `<use>` that does render is a 0×0 fallback. This is the classic icon shape (`<defs><path/></defs><use/>`) and it silently violates the documented "no node is dropped silently … rather than silent mis-conversion" contract (the doc's justification, "`defs` … carries definitions, not artwork", argues for the opposite of what the code does). Penpot's own importer does not do this: `csvg/extract-defs` pulls defs out of the content tree into the root shape's `:svg-defs` (`/tmp/penpot-src/common/src/app/common/files/shapes_builder.cljc:216,272`), never as shapes. Suggested fix: stop recursing into `defs` with the normal visiting context — either record the whole `<defs>` subtree as a single `svg-raw` skip (so the markup is preserved without being rendered), or drop `defs` from `TRANSPARENT_ELEMENTS` and give it its own `unsupported element "defs"` reason; then update the test at `svg.test.ts` ("convertSvg traverses defs transparently and reports its contents") to assert the defs content produces **no** native shape. ## High - [x] `extensions/penpot/src/svg.ts:1474` — non-rendering state is ignored: `<rect display="none">`, `visibility="hidden"` and `opacity="0"` all produce an ordinary, fully visible shape (verified), because `visit` only guards `UNSUPPORTED_ATTRIBUTES = ["mask","filter","clip-path"]`. `display:none` means "do not render this element (nor its children)", so a hidden layer in an exported SVG reappears as visible artwork — the same silent-mis-conversion class as the `defs` bug, and undocumented in the module's "Known limitations". Penpot's importer maps it explicitly (`(= (dm/get-in shape [:svg-attrs :display]) "none") … (assoc :hidden true)`, `shapes_builder.cljc:640-650`), and Penpot's shape schema has `:hidden` and `:opacity`. Suggested fix: read `display`/`visibility`/`opacity` (attribute and inline `style`) in `mergeStyle`; when `display:none` (or `visibility:hidden`, `opacity:0`) either carry a `hidden`/`opacity` field on `ShapeDescriptor` for the tool step (Penpot has both) or route the node to `svg-raw` with a specific reason — and add a test for each. - [x] `extensions/penpot/src/svg.ts:1115` — `parseNumber` uses `Number.parseFloat`, which silently ignores a unit suffix, so unit-bearing lengths are mis-scaled instead of falling back: `<rect width="50%" height="50%"/>` becomes a 50×50 rect, `x="10pt"` → 10, `width="1cm"` → 1, `font-size="12pt"` → 12 (all verified, no `skipped` entry). Only `rootMatrix`'s `parseAbsoluteLength` is unit-aware, and it special-cases `%` only. This is silently wrong output for a documented-subset input, and the reference implementation resolves it rather than ignoring it (Penpot's `csvg/fix-percents`, `/tmp/penpot-src/common/src/app/common/svg.cljc:950`, scales `%` against the viewBox). Suggested fix: parse length values with an explicit unit check (`""`/`px` accepted; `%` resolved against the viewBox/root matrix; every other unit — `em ex cm mm pt pc` — a fallback with a reason such as `unsupported length unit "pt"`), and test a `%`-sized rect and a `pt` font-size. - [x] `extensions/penpot/src/svg.test.ts:1` — several implemented branches of the new converter have no test, including the only occurrences of two distinct fallback reasons: `applyMatrixToRect`'s rounded-rect path (`rx`/`ry` → `geometry.radius`) and both `REASON_ELLIPTICAL_RADIUS` refusals (`svg.ts:804-820`); `preserveAspectRatio="none"` (`svg.ts:1298`, a documented feature); `stroke-linecap="square"` and the omitted-`butt` case (`svg.ts:1244`); `geometryIsConsistent` returning `false` for any kind (`svg.ts:1000`); and every unit-suffixed length (see the High finding above). Suggested fix: add focused tests — a `rx="2"` rect asserting `geometry.radius === 2`, a `rx="2" ry="4"` rect and a `scale(2 1)` rounded rect asserting the elliptical-radius fallback, a non-square `viewBox` with `preserveAspectRatio="none"`, `stroke-linecap="square"` plus a `stroke-linecap="butt"` stroke with no `strokeCapStart` key, a hand-built inconsistent geometry asserting `false`, and a `%`/`pt` geometry fixture. ## Medium - [x] `extensions/penpot/src/svg.ts:352` — `argsText.split(/[\s,]+/).map(Number)` turns empty argument fields into `0`, so malformed transform syntax is **accepted** instead of failing as the module docs promise ("Malformed syntax is an explicit failure"): `matrixFrom("scale(,)")` → `{a:0,d:0}` (a silently collapsed zero-scale shape), `matrixFrom("translate(,10)")` → `{e:0,f:10}`, `matrixFrom("matrix(1,,0,0,1,5 6)")` → accepted (all verified). Suggested fix: split first and reject any empty field (`if (parts.some(p => p.trim() === "")) return invalidTransform(value)`), then `Number()` the parts; add tests for the three cases above. - [x] `extensions/penpot/src/svg.ts:1652` — `parseNumber(node.attributes["font-weight"], 400)` silently returns the 400 fallback for the keyword forms, so the very common `font-weight="bold"` is emitted as `fontWeight: 400` with no fallback and no report entry (verified). Suggested fix: map `normal|bold|bolder|lighter` (and `font-style` if it is ever used) before falling back, or push the node to `svg-raw` with `unsupported font-weight "bold"`; add a test. - [x] `extensions/penpot/src/svg.ts:1712` — `pushSkip` derives `svg-raw` geometry from `localBounds`, which returns `zeroBounds()` for `use`, `image`, `mask`, a nested `svg` and anything unrecognised, so the most common fallback (present in the tests, e.g. `<use href="#x"/>`) is emitted as a 0×0 box — preserved markup the design will not show, and `SkippedNode` gives the caller no way to tell "preserved but invisible" from "preserved with real bounds". Suggested fix: resolve `<use href="#id">` against the document's ids (and `image` against its `width`/`height`) for the fallback bounds, and/or add a `zeroGeometry: true` flag to `SkippedNode` (and a distinct reason) so the tool step can surface it. - [x] `extensions/penpot/src/xml.ts:390` — mixed content loses its ordering, because `XmlNode.text` accumulates all direct text while children are kept separately and `serializeXml` re-emits text first: `<text x="0" y="10">a<tspan>b</tspan>c</text>` is preserved as `<text x="0" y="10">ac<tspan>b</tspan></text>` (verified), i.e. the `svg-raw` fallback's markup — its whole reason for existing — is wrong. Suggested fix: represent text as positioned nodes (`content: Array<string | XmlNode>`) and serialise in order, or, if that is out of scope, document the limitation and make `serializeXml` reject/flag mixed content rather than silently reorder it; add a test. - [x] `extensions/penpot/src/svg.ts:44` — the module doc states "The reader is bounded by {@link DEFAULT_MAX_NODES} visited elements", but `parseXml` has no element-count bound (only `MAX_XML_DEPTH`), and the node budget is enforced during `convertSvg`'s traversal **after** the whole document is parsed and materialised — so a hostile 10 M-element SVG is fully parsed into memory before `SvgError` is thrown (verified: 20 000 rects throw, but only after parsing). Suggested fix: correct the doc to say the *traversal* is bounded and the reader is bounded by depth only, or move the count into `parseXml` via an optional `maxNodes` so the bound holds before materialisation. - [x] `CHANGELOG.md:7` — issue #209 records nothing: the commit touches only the five source/test files, with no `## [Unreleased]` entry and no `extensions/penpot/findings.md` section, while every prior issue in this extension did both — including the tool-less pure module of issue #202 ("Binfile Transit decoder + SSE parser — issue #202") and the source-verified pass of #208. Suggested fix: add a CHANGELOG `### Added` entry describing the converter and XML reader (scope, documented subset, `svg-raw` fallback, 79 tests) and a `## SVG → native shapes converter — issue #209` section in `extensions/penpot/findings.md` recording the subset decisions and the two Penpot-2.17.2 comparisons above (defs extraction, `display:none` → `hidden`). ## Low - [x] `extensions/penpot/src/svg.ts:210` — the `svg-raw` descriptor carries `raw` as a serialised markup **string**, but Penpot's `svg-raw` shape does not accept one: `create-raw-svg` stores `content` as hiccup data plus `svg-attrs` and `svg-viewbox` (`/tmp/penpot-src/common/src/app/common/files/shapes_builder.cljc:270-284`). The "ready for the tool-wrapping step" claim therefore cannot be honoured by passing `raw` through. Suggested fix: document on `ShapeDescriptor.raw` (and in the module doc) that the tool step must re-parse it into Penpot's `content`/`svg-attrs`/`svg-viewbox`, or emit that structure here. - [x] `extensions/penpot/src/xml.ts:321` — `codePointToString` accepts any code point in `[0, 0x10ffff]`, so `` yields `"\0"` and `&#xD800;` yields a lone surrogate (verified) — both illegal in XML and both able to reach preserved `svg-raw` markup. Suggested fix: also reject the C0 controls except tab/newline/carriage-return, `0xFFFE`/`0xFFFF`, and the surrogate range `0xD800–0xDFFF`. - [x] `extensions/penpot/src/index.ts:1` — the converter is not imported anywhere in the extension (only `svg.test.ts` references it), so nothing yet surfaces the `report`/`skipped` accounting to the agent, which is exactly what plan M6 asks the tool step to do ("Record what was converted vs. fell back, and surface that to the agent"). Suggested fix: if a follow-up issue owns `penpot_add_svg`, say so in the CHANGELOG/findings entry (see the Medium finding); otherwise the converter is unreachable code and that milestone remains open.

david added 2 commits

2026-09-15 06:12:30 +00:00