issue-252: penpot_add_text stages zero-area auto-width text, producing files that fail update-file with NaN points #265

Merged

david merged 2 commits from feature/issue-252/penpot-add-text-stages-zero-area-auto-width-text-p into main

2026-09-17 04:41:02 +00:00

david commented

2026-09-17 04:41:01 +00:00

Owner

252: penpot_add_text stages zero-area auto-width text, producing files that fail update-file with NaN points

Closes #252 — 252.

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 1h 03m 49s.

Review findings

I'll start by reading the relevant files for the findings.Now let me look at the specific files mentioned in the findings.Let me look at more details — the instance change payloads, imports in svg.ts, and test assertions on "zero-area".Now let me check imports and the discard tool, and whether svg.ts can import from shapes.ts without a cycle.Now let me check how instance.ts sets shapeRef/componentId and what types the change obj carries:Now let me check the discard tool name, boundsToBox, and the commit test helpers:Now let me check instance.ts typing for shapeRef/componentId and the add_svg test harness:Now let me look at the commit test helpers and the isAxisAligned function:Let me look at the dry_run tests in commit.test.ts and the svg test harness helpers:Let me check DEFAULT_TEXT_STYLE and remaining usages, then start making fixes.I have the full picture now. Let me check the svg tool test harness before making edits:I have full context now. Let me check the svg.ts module docs for text documentation, then make the edits.Now I have full context. Let me check the svg tool test helpers:I now have complete context. Let me apply the fixes, starting with shapes.ts (Low findings: assertPositiveBox wording and shared constant):Now fix the commit.ts findings (Medium: dry_run ordering; Low: instance remedy):The componentId field isn't on ShapePayload yet — instance clones carry it via a cast. Let me add the optional fields so the access is type-sound:Now the Medium + Low findings in svg.ts — guard the non-positive font-size via pushSkip and share the glyph-width constant:Now update the commit pre-flight test (dry_run behaviour) and add tests. First let me update the existing commit test to also pin the dry-run refusal:Now add tests for the SVG font-size guard in svg.test.ts and the tools/svg.test.ts:Let me fix the doc comment on assertPositiveBox (it still says "zero-area" is avoided, but the message keeps the phrase plus the ≤ 0 breakdown), then add the negative-dimension test:Now add the negative-dimension test in shapes.test.ts:Now run the test suite:All 880 tests pass (879 pass, 1 live-only skip, 0 fail). Let me update the CHANGELOG entry to stay consistent with the repo's convention, and verify no stale references remain:All gates green: 880 tests, 879 pass, 1 live-only skip, 0 fail. Let me do a final sanity check of the working-tree changes:All remediation work is complete and gates are green. Here is the annotated findings document:

Review Findings — 252

Summary

Fixed all five code findings in one pass: the SVG import's convertText now routes a non-positive parsed font-size through pushSkip (so a zero-area text descriptor is never emitted and the staging loop stays atomic), penpot_commit's text-geometry pre-flight now runs before the dry_run branch (so a dry run refuses an uncommittable batch instead of reporting it committable), the zero-area refusal branches its remedy on the change carrying componentId/shapeRef (naming the library component for penpot_instance_component clones), the glyph-width factor is now one shared constant (APPROXIMATE_CHARACTER_WIDTH, exported from shapes.ts and used by the SVG converter), and assertPositiveBox reports failing dimensions with their ≤ 0 comparison so a negative width/height isn't misread as a plain zero-size box. New tests pin the SVG skip (converter- and tool-level, including staging atomicity), the dry-run refusal, the instance-clone remedy, and the negative-dimension message. Gates: npm test — 880 tests, 879 pass, 1 live-only skip, 0 fail; the repo has no build or lint script. The commit-message finding is left for the squash-merge step: amending d4e7654 would require a commit, which this stage must not perform.

Critical

None.

High

None.

Medium

extensions/penpot/src/tools/svg.ts:1002 — The SVG-import text path (convertText in src/svg.ts:1995 computes width = text.length * fontSize * TEXT_WIDTH_FACTOR) can still produce a zero-area text descriptor (e.g. font-size="0" or a negative font-size, both of which parseLength accepts), and buildShapeForDescriptor → baseShape now throws mid-staging-loop. Because stageChange is called per descriptor inside the loop with no try/catch, descriptors staged before the throw remain in the changeset and the tool exits with a raw internal error instead of its formatted skip report. Suggested fix: in convertText (svg.ts:~1965), route a non-positive parsed fontSize through pushSkip like the other invalid-input cases, or estimate a positive box via estimateTextSize for zero-area text descriptors so the import stays atomic.

Low

extensions/penpot/src/tools/commit.ts:527 — assertCommittableTextGeometry runs after the dry_run === true early return, so penpot_commit with dry_run: true reports a batch containing a degenerate text shape as committable while the real commit will refuse it. Suggested fix: run the pre-flight before the dry-run branch (it performs no I/O, so dry-run semantics are unaffected) or surface the refusal in dryRunText.
extensions/penpot/src/tools/commit.ts:441 — The zero-area refusal's remedy ("Re-stage the text with penpot_add_text (omitted dimensions are estimated)…") doesn't apply to text shapes that arrive via penpot_instance_component clones (instance.ts does not go through baseShape), where the correct remedy is discarding the instance or fixing the library. Suggested fix: branch the remedy on whether the change carries shapeRef/componentId, or add a generic "or discard the staged change" that covers the instance case first.
extensions/penpot/src/shapes.ts:902 vs extensions/penpot/src/svg.ts:1194 — Two near-identical glyph-width constants now exist (APPROXIMATE_CHARACTER_WIDTH = 0.58 and TEXT_WIDTH_FACTOR = 0.6), which will drift. Suggested fix: have the SVG text path reuse estimateTextSize (also resolving the Medium finding) or export and share one constant.
extensions/penpot/src/shapes.ts:412 — assertPositiveBox reports negative dimensions as a "zero-area box (width -5, …)"; the message is slightly misleading for the negative case. Suggested fix: word it "non-positive box" or report width ${input.width} ≤ 0.
Commit d4e7654 — commit message is 252: … but the AGENTS.md convention for issue-driven work is issue-252: … (branch name feature/issue-252/… is correct). Suggested fix: use issue-252: <summary> for the squash-merge message when the PR is merged.

(All severities assessed against the repo's own convention docs; no acceptance criteria were provided beyond the issue title, which the change addresses.)

## 252: penpot_add_text stages zero-area auto-width text, producing files that fail update-file with NaN points Closes #252 — [252](https://git.excelera.net/david/pi-extensions-and-skills/issues/252). > 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 1h 03m 49s. ## Review findings I'll start by reading the relevant files for the findings.Now let me look at the specific files mentioned in the findings.Let me look at more details — the instance change payloads, imports in svg.ts, and test assertions on "zero-area".Now let me check imports and the discard tool, and whether svg.ts can import from shapes.ts without a cycle.Now let me check how instance.ts sets `shapeRef`/`componentId` and what types the change obj carries:Now let me check the discard tool name, `boundsToBox`, and the commit test helpers:Now let me check instance.ts typing for `shapeRef`/`componentId` and the add_svg test harness:Now let me look at the commit test helpers and the `isAxisAligned` function:Let me look at the dry_run tests in commit.test.ts and the svg test harness helpers:Let me check `DEFAULT_TEXT_STYLE` and remaining usages, then start making fixes.I have the full picture now. Let me check the svg tool test harness before making edits:I have full context now. Let me check the svg.ts module docs for text documentation, then make the edits.Now I have full context. Let me check the svg tool test helpers:I now have complete context. Let me apply the fixes, starting with `shapes.ts` (Low findings: `assertPositiveBox` wording and shared constant):Now fix the commit.ts findings (Medium: dry_run ordering; Low: instance remedy):The `componentId` field isn't on `ShapePayload` yet — instance clones carry it via a cast. Let me add the optional fields so the access is type-sound:Now the Medium + Low findings in `svg.ts` — guard the non-positive font-size via `pushSkip` and share the glyph-width constant:Now update the commit pre-flight test (dry_run behaviour) and add tests. First let me update the existing commit test to also pin the dry-run refusal:Now add tests for the SVG font-size guard in svg.test.ts and the tools/svg.test.ts:Let me fix the doc comment on `assertPositiveBox` (it still says "zero-area" is avoided, but the message keeps the phrase plus the `≤ 0` breakdown), then add the negative-dimension test:Now add the negative-dimension test in shapes.test.ts:Now run the test suite:All 880 tests pass (879 pass, 1 live-only skip, 0 fail). Let me update the CHANGELOG entry to stay consistent with the repo's convention, and verify no stale references remain:All gates green: 880 tests, 879 pass, 1 live-only skip, 0 fail. Let me do a final sanity check of the working-tree changes:All remediation work is complete and gates are green. Here is the annotated findings document: # Review Findings — 252 ## Summary Fixed all five code findings in one pass: the SVG import's `convertText` now routes a non-positive parsed `font-size` through `pushSkip` (so a zero-area text descriptor is never emitted and the staging loop stays atomic), `penpot_commit`'s text-geometry pre-flight now runs before the `dry_run` branch (so a dry run refuses an uncommittable batch instead of reporting it committable), the zero-area refusal branches its remedy on the change carrying `componentId`/`shapeRef` (naming the library component for `penpot_instance_component` clones), the glyph-width factor is now one shared constant (`APPROXIMATE_CHARACTER_WIDTH`, exported from `shapes.ts` and used by the SVG converter), and `assertPositiveBox` reports failing dimensions with their `≤ 0` comparison so a negative width/height isn't misread as a plain zero-size box. New tests pin the SVG skip (converter- and tool-level, including staging atomicity), the dry-run refusal, the instance-clone remedy, and the negative-dimension message. Gates: `npm test` — 880 tests, 879 pass, 1 live-only skip, 0 fail; the repo has no build or lint script. The commit-message finding is left for the squash-merge step: amending `d4e7654` would require a commit, which this stage must not perform. ## Critical - [x] None. ## High - [x] None. ## Medium - [x] extensions/penpot/src/tools/svg.ts:1002 — The SVG-import text path (`convertText` in `src/svg.ts:1995` computes `width = text.length * fontSize * TEXT_WIDTH_FACTOR`) can still produce a zero-area text descriptor (e.g. `font-size="0"` or a negative font-size, both of which `parseLength` accepts), and `buildShapeForDescriptor` → `baseShape` now throws mid-staging-loop. Because `stageChange` is called per descriptor inside the loop with no try/catch, descriptors staged before the throw remain in the changeset and the tool exits with a raw internal error instead of its formatted skip report. Suggested fix: in `convertText` (svg.ts:~1965), route a non-positive parsed `fontSize` through `pushSkip` like the other invalid-input cases, or estimate a positive box via `estimateTextSize` for zero-area text descriptors so the import stays atomic. ## Low - [x] extensions/penpot/src/tools/commit.ts:527 — `assertCommittableTextGeometry` runs *after* the `dry_run === true` early return, so `penpot_commit` with `dry_run: true` reports a batch containing a degenerate text shape as committable while the real commit will refuse it. Suggested fix: run the pre-flight before the dry-run branch (it performs no I/O, so dry-run semantics are unaffected) or surface the refusal in `dryRunText`. - [x] extensions/penpot/src/tools/commit.ts:441 — The zero-area refusal's remedy ("Re-stage the text with penpot_add_text (omitted dimensions are estimated)…") doesn't apply to text shapes that arrive via `penpot_instance_component` clones (instance.ts does not go through `baseShape`), where the correct remedy is discarding the instance or fixing the library. Suggested fix: branch the remedy on whether the change carries `shapeRef`/`componentId`, or add a generic "or discard the staged change" that covers the instance case first. - [x] extensions/penpot/src/shapes.ts:902 vs extensions/penpot/src/svg.ts:1194 — Two near-identical glyph-width constants now exist (`APPROXIMATE_CHARACTER_WIDTH = 0.58` and `TEXT_WIDTH_FACTOR = 0.6`), which will drift. Suggested fix: have the SVG text path reuse `estimateTextSize` (also resolving the Medium finding) or export and share one constant. - [x] extensions/penpot/src/shapes.ts:412 — `assertPositiveBox` reports negative dimensions as a "zero-area box (width -5, …)"; the message is slightly misleading for the negative case. Suggested fix: word it "non-positive box" or report `width ${input.width} ≤ 0`. - [ ] Commit `d4e7654` — commit message is `252: …` but the AGENTS.md convention for issue-driven work is `issue-252: …` (branch name `feature/issue-252/…` is correct). Suggested fix: use `issue-252: <summary>` for the squash-merge message when the PR is merged. *(All severities assessed against the repo's own convention docs; no acceptance criteria were provided beyond the issue title, which the change addresses.)*

david added 2 commits

2026-09-17 04:41:01 +00:00