issue-207: Implement penpot_instance_component with id remapping #240

Merged

david merged 3 commits from feature/issue-207/implement-penpot-instance-component-with-id-remapp into main

2026-09-15 05:31:21 +00:00

david commented

2026-09-15 05:31:20 +00:00

Owner

207: Implement penpot_instance_component with id remapping

Closes #207 — 207.

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 16m 59s.

Review findings

Now let's begin the fixes. First, the pure core (High 1) — stop transmitting the child shapes list:All fixes are applied and the penpot suite (including the new integration test) is green. The full npm test reproduces only the documented pre-existing failure. Here is the annotated findings document.

Review Findings — 207

Summary

All eight findings were addressed in a single pass. The clone no longer transmits each container's remapped shapes list (it sends shapes: [] and lets the server's add-shape maintain the child list), with the pure assertions and a new instanceCommit.test.ts integration test updated to pin the applied tree; a CHANGELOG.md entry was added; the dead unlinkedLibraryMessage path was replaced with an internal-invariant error and the docs/description reworded; the missing integration coverage and open live gap were recorded in findings.md; and the four Low findings (unused import, dead LibrarySource.name, duplicated name-bounding helper, and the misleading "library file" label for local components) were resolved. npm test stays green apart from the pre-existing, unrelated scripts/local-install.test.mjs:1875 failure documented in the review.

Critical

none

High

extensions/penpot/src/instance.ts:351 — The clone forwards each source frame's shapes array into the staged add-obj payload, and the children are then staged as their own add-obj changes. Penpot's server-side add-shape appends a child id to its parent's shapes list when the child's add-obj is applied (that is the behaviour the repo's own stub model documents), so if it does not deduplicate, every child id lands in the parent list twice. This is the only composition primitive that sends a non-empty shapes array (baseShape always emits shapes: [] for frames), and the shared stub silently dedupes (if (!shapes.includes(...))), so neither the pure tests nor composedScreen.test.ts can detect a double-append — and there is no end-to-end validation of this payload. Suggested fix: do not transmit the remapped child list (emit shapes: []/delete shapes and let Penpot build the list as the children are added, matching penpot_add_frame), and update the pure assertions that pin root.obj.shapes/panel.obj.shapes. If the pre-populated array is in fact required, pin the dedupe behaviour with an integration test (see below) before merging.
CHANGELOG.md:9 — No entry was added under ## [Unreleased] / ### Added. Every prior penpot tool-landing commit in this repo (e.g. 032e677, 4ff820c, ba1fb50) updated CHANGELOG.md as part of the same change; this one registers a new tool and updates the README/SKILL docs but leaves the changelog silent. Suggested fix: add an ### Added bullet describing penpot_instance_component (component resolution by name across the file and its linked libraries, main-instance subtree clone with fresh ids and shapeRef, componentId/componentFile/componentRoot on the root, positioning at x/y, the refusal paths, and the read-only-until-commit guarantee), matching the style of the neighbouring entries.

Medium

extensions/penpot/src/tools/instance.ts:476 — unlinkedLibraryMessage is unreachable: component is only ever resolved from candidates, which is built from ownComponents (all with fileId === params.file_id) plus the successfully-read sources; so when component.fileId !== params.file_id the sources.find(...) lookup can never return undefined. The dead branch also backs a documentation claim that does not exist in behaviour (the README/SKILL/description say an unlinked library is "refused with a specific remedy"; in practice an unlinked component is reported by the generic missing-name message, which merely appends LINK_LIBRARY_HINT). Suggested fix: delete unlinkedLibraryMessage and replace the source === undefined branch with an internal-invariant error, or reword the docs to say the unlinked case is surfaced through the missing-name message with the link hint; add a test that actually exercises whatever path is kept.
extensions/penpot/findings.md:1204 — Component instancing has no end-to-end coverage and the live-validation gap is not recorded. The tool's tests stub globalThis.fetch with canned get-file bodies, so the produced instance payloads are never applied to the shared stateful Penpot model (src/tools/fixtures/penpotStubModel.ts) that validates required keys and parent/shapes maintenance; the design/implementation plan lists component instancing as an integration item, and every comparable tool change (issues #201, #205, #206) recorded its validation state or gaps in findings.md. Suggested fix: add an integration test (analogous to composedScreen.test.ts) that drives instanceComponentTool against the shared stub model — a consumer file plus a linked library holding a component and its main instance — then commits and asserts the applied tree; and add a "Still open" note to findings.md stating the instancing payload was never accepted by a real server.

Low

extensions/penpot/src/tools/instance.ts:56 — StagedChangeToolResult is imported but never used (the result type is declared from AgentToolResult<InstanceComponentToolDetails> here, whereas frame.ts/rect.ts use it as AddXToolResult = StagedChangeToolResult & {…}). Suggested fix: either remove the unused import or adopt the sibling tools' pattern (export type InstanceComponentToolResult = StagedChangeToolResult & { details: InstanceComponentToolDetails }) for consistency.
extensions/penpot/src/tools/instance.ts:120 — LibrarySource.name is populated (line 227) but never read anywhere in the tool; it is dead state. Suggested fix: remove the field and the ...(entry.name === undefined ? {} : { name: entry.name }) spread, or use the library name in the refusal/note text (e.g. library "<name>" [id]).
extensions/penpot/src/tools/instance.ts:238 — availableComponentNames re-derives the distinct, sorted, bounded name list that resolveAsset already returns in the missing failure (failure.available + failure.truncated), so the truncation/bounding rules are duplicated and can drift. Suggested fix: pass resolved.available/resolved.truncated into missingComponentMessage (or accept the ResolveFailure) and delete the helper.
extensions/penpot/src/tools/instance.ts:315 — missingMainInstancePageMessage (and the sibling at line 328) always says "library file [id]" even when the owning file is the consumer's own file for a local component, which points the agent at a non-existent library. Suggested fix: derive the label the same way the success note does (component.fileId === params.file_id ? "this file" : \library [id]``) and phrase both errors with it.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md
docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 207: Implement penpot_instance_component with id remapping Closes #207 — [207](https://git.excelera.net/david/pi-extensions-and-skills/issues/207). > 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 16m 59s. ## Review findings Now let's begin the fixes. First, the pure core (High 1) — stop transmitting the child shapes list:All fixes are applied and the penpot suite (including the new integration test) is green. The full `npm test` reproduces only the documented pre-existing failure. Here is the annotated findings document. # Review Findings — 207 ## Summary All eight findings were addressed in a single pass. The clone no longer transmits each container's remapped `shapes` list (it sends `shapes: []` and lets the server's `add-shape` maintain the child list), with the pure assertions and a new `instanceCommit.test.ts` integration test updated to pin the applied tree; a `CHANGELOG.md` entry was added; the dead `unlinkedLibraryMessage` path was replaced with an internal-invariant error and the docs/description reworded; the missing integration coverage and open live gap were recorded in `findings.md`; and the four Low findings (unused import, dead `LibrarySource.name`, duplicated name-bounding helper, and the misleading "library file" label for local components) were resolved. `npm test` stays green apart from the pre-existing, unrelated `scripts/local-install.test.mjs:1875` failure documented in the review. ## Critical - none ## High - [x] `extensions/penpot/src/instance.ts:351` — The clone forwards each source frame's `shapes` array into the staged `add-obj` payload, and the children are then staged as their own `add-obj` changes. Penpot's server-side `add-shape` appends a child id to its parent's `shapes` list when the child's `add-obj` is applied (that is the behaviour the repo's own stub model documents), so if it does not deduplicate, every child id lands in the parent list twice. This is the only composition primitive that sends a non-empty `shapes` array (`baseShape` always emits `shapes: []` for frames), and the shared stub silently dedupes (`if (!shapes.includes(...))`), so neither the pure tests nor `composedScreen.test.ts` can detect a double-append — and there is no end-to-end validation of this payload. Suggested fix: do not transmit the remapped child list (emit `shapes: []`/delete `shapes` and let Penpot build the list as the children are added, matching `penpot_add_frame`), and update the pure assertions that pin `root.obj.shapes`/`panel.obj.shapes`. If the pre-populated array is in fact required, pin the dedupe behaviour with an integration test (see below) before merging. - [x] `CHANGELOG.md:9` — No entry was added under `## [Unreleased] / ### Added`. Every prior penpot tool-landing commit in this repo (e.g. `032e677`, `4ff820c`, `ba1fb50`) updated `CHANGELOG.md` as part of the same change; this one registers a new tool and updates the README/SKILL docs but leaves the changelog silent. Suggested fix: add an `### Added` bullet describing `penpot_instance_component` (component resolution by name across the file and its linked libraries, main-instance subtree clone with fresh ids and `shapeRef`, `componentId`/`componentFile`/`componentRoot` on the root, positioning at `x`/`y`, the refusal paths, and the read-only-until-commit guarantee), matching the style of the neighbouring entries. ## Medium - [x] `extensions/penpot/src/tools/instance.ts:476` — `unlinkedLibraryMessage` is unreachable: `component` is only ever resolved from `candidates`, which is built from `ownComponents` (all with `fileId === params.file_id`) plus the successfully-read `sources`; so when `component.fileId !== params.file_id` the `sources.find(...)` lookup can never return `undefined`. The dead branch also backs a documentation claim that does not exist in behaviour (the README/SKILL/description say an unlinked library is "refused with a specific remedy"; in practice an unlinked component is reported by the generic missing-name message, which merely appends `LINK_LIBRARY_HINT`). Suggested fix: delete `unlinkedLibraryMessage` and replace the `source === undefined` branch with an internal-invariant error, or reword the docs to say the unlinked case is surfaced through the missing-name message with the link hint; add a test that actually exercises whatever path is kept. - [x] `extensions/penpot/findings.md:1204` — Component instancing has no end-to-end coverage and the live-validation gap is not recorded. The tool's tests stub `globalThis.fetch` with canned `get-file` bodies, so the produced instance payloads are never applied to the shared stateful Penpot model (`src/tools/fixtures/penpotStubModel.ts`) that validates required keys and parent/`shapes` maintenance; the design/implementation plan lists component instancing as an integration item, and every comparable tool change (issues #201, #205, #206) recorded its validation state or gaps in `findings.md`. Suggested fix: add an integration test (analogous to `composedScreen.test.ts`) that drives `instanceComponentTool` against the shared stub model — a consumer file plus a linked library holding a component and its main instance — then commits and asserts the applied tree; and add a "Still open" note to `findings.md` stating the instancing payload was never accepted by a real server. ## Low - [x] `extensions/penpot/src/tools/instance.ts:56` — `StagedChangeToolResult` is imported but never used (the result type is declared from `AgentToolResult<InstanceComponentToolDetails>` here, whereas `frame.ts`/`rect.ts` use it as `AddXToolResult = StagedChangeToolResult & {…}`). Suggested fix: either remove the unused import or adopt the sibling tools' pattern (`export type InstanceComponentToolResult = StagedChangeToolResult & { details: InstanceComponentToolDetails }`) for consistency. - [x] `extensions/penpot/src/tools/instance.ts:120` — `LibrarySource.name` is populated (line 227) but never read anywhere in the tool; it is dead state. Suggested fix: remove the field and the `...(entry.name === undefined ? {} : { name: entry.name })` spread, or use the library name in the refusal/note text (e.g. `library "<name>" [id]`). - [x] `extensions/penpot/src/tools/instance.ts:238` — `availableComponentNames` re-derives the distinct, sorted, bounded name list that `resolveAsset` already returns in the `missing` failure (`failure.available` + `failure.truncated`), so the truncation/bounding rules are duplicated and can drift. Suggested fix: pass `resolved.available`/`resolved.truncated` into `missingComponentMessage` (or accept the `ResolveFailure`) and delete the helper. - [x] `extensions/penpot/src/tools/instance.ts:315` — `missingMainInstancePageMessage` (and the sibling at line 328) always says "library file [id]" even when the owning file is the consumer's own file for a local component, which points the agent at a non-existent library. Suggested fix: derive the label the same way the success note does (`component.fileId === params.file_id ? "this file" : \`library [id]\``) and phrase both errors with it. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-15 05:31:20 +00:00