issue-208: Validate component instancing against the live instance #241

Merged

david merged 3 commits from feature/issue-208/validate-component-instancing-against-the-live-ins into main

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

david commented

2026-09-15 05:49:30 +00:00

Owner

208: Validate component instancing against the live instance

Closes #208 — 208.

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 18m 08s.

Review findings

I'll start by exploring the repository state and the relevant files.Applied the feasible findings, re-ran the gates (stub harness 19/19, node --test 688 tests with only the pre-existing unrelated scripts/local-install.test.mjs:1875 failure), and verified the new harness checks are non-vacuous by temporarily reverting the strip.

Review Findings — 208

Summary

The diff is a validation change for penpot_instance_component: it strips touched/variantId/variantName from each clone in buildInstance (matching Penpot 2.17.2's make-component-instance), adds a unit regression test, adds harness check 8 plus a stub library Button, and documents the pass in findings.md/README.md/CHANGELOG.md. Gates: build — none configured (no root tsconfig/build step; TS is type-stripped by node --test and the pi loader); lint/typecheck — none configured (no eslint/prettier/biome/tsc in the repo); test — npm test runs the stub harness (check 8 passes 16/16) plus node --test: 682 pass, 1 fail, and the failure is pre-existing and unrelated (scripts/local-install.test.mjs:1875 asserts extensions/mongodb/index.ts is in package.json's pi.extensions; it is absent and neither file is touched by this diff — confirmed against git show bbd1ae3:package.json). The penpot suites alone are 556 pass / 0 fail. The substantive gap is that the issue's own deliverable — validation against the live instance — did not happen; the static comparison it did perform is sound but missed at least one further deviation from Penpot's clone (swap-keep attrs), which I reproduced locally.

Remediation: fixed all of High, Medium and Low. buildInstance now removes the root's swap-keep attributes (restoring only an auto sizing when the root drives a layout), sets ignoreTouched: true on every change, and strips mainInstance from every clone; harness check 8 asserts the three stripped instance-state fields and compares asset refs against the main shape, and the stub's Button root now carries touched/variantId/variantName so the strip is exercised end to end (19/19, and reverting the deletes makes those checks fail); the findings.md "never accepted" claim is qualified and the PENPOT_SKILL_DESIGN.md/PENPOT_SKILL_IMPLEMENTATION_PLAN.md record is reconciled. The parent-inside-a-copy part of the ignoreTouched finding is explicitly documented (it needs the target file's shape tree, which the pure function does not receive) rather than half-implemented. The Critical finding remains: this environment has no PENPOT_URL/PENPOT_TOKEN and no browser, so the live validation cannot be performed here.

Critical

extensions/penpot/findings.md:1277 — the issue ("Validate component instancing against the live instance") is not delivered: no live write, no live get-file re-read and no UI observation was performed, so componentId/componentFile/componentRoot/shapeRef, main-component propagation, override survival and unlink behaviour remain unverified against the real server. Every client-visible claim in the same document is the version from my earlier investigation and I have not independently re-checked it."). Suggested fix: run PENPOT_URL=… PENPOT_TOKEN=… PENPOT_SCRATCH_FILE_ID=… PENPOT_LIBRARY_FILE_ID=… node extensions/penpot/scripts/live-write-check.mjs (check 8) from an environment that has the token — the 2026-09-14 pass recorded in docs/PENPOT_SKILL_DESIGN.md:329 and docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md:9 shows the live instance and a token have been available outside the implement environment — then execute and record the printed UI checklist in findings.md; if that is genuinely impossible, mark the issue blocked/validated-by-static-analysis-only rather than closing it as validated.

High

extensions/penpot/src/instance.ts:380 — the clone still keeps the root's swap-keep attributes (layoutItemMargin, layoutItemMarginType, layoutItemHSizing, layoutItemVSizing, layoutItemMaxH/MinH/MaxW/MinW, layoutItemAbsolute, layoutItemZIndex, layoutItemAlignSelf, interactions), which Penpot's own instantiate path deliberately removes from the component root before cloning (remove-swap-keep-attrs, common/src/app/common/types/container.cljc:287-298, applied at :321). Reproduced with buildInstance on a main root carrying layoutItemAbsolute: true + interactions: both survive into the instance root. Consequence: an instance dropped into an auto-layout frame inherits the library's absolute positioning/margins/z-index instead of behaving as a fresh child, and interactions copied into the consumer file reference shape ids that belong to the library file. Suggested fix: in the isRoot branch delete the twelve swap-keep keys from cloned, then re-set layoutItemHSizing/layoutItemVSizing to "auto" only when the library root has a layout (any-layout?) and that sizing was "auto" — mirroring remove-swap-keep-attrs — and add a instance.test.ts case asserting the root clone drops the non-auto ones while a child's layoutItem* is preserved.

Medium

extensions/penpot/src/instance.ts:389 — the emitted add-obj changes carry no ignoreTouched, while Penpot's own instantiate adds every instance shape with {:ignore-touched true} (common/src/app/common/logic/libraries.cljc:287,314; schema changes.cljc:199; the flag is consumed by ctst/add-shape → shape_tree.cljc:42 to keep :remote-synced on a parent that is itself a component copy). Relatedly, generate-instantiate-component dissocs :component-root from the instance root when the target parent is inside a component copy (libraries.cljc:293-295), which buildInstance never does. Consequence: instantiating into a frame that is part of a component copy marks that copy as locally modified (and produces a nested component-root Penpot would not), so the copy stops syncing. Suggested fix: add ignoreTouched: true to each returned change in buildInstance, and either handle or explicitly document the parent-inside-a-copy case (drop componentRoot when the resolved parent_id's ancestors include a shape-ref).
extensions/penpot/findings.md:1286 — "The instance payload was never accepted by a real Penpot server" contradicts the repository's own record: docs/PENPOT_SKILL_DESIGN.md:329 ("Component instancing | Created a component from a frame, then built a working instance (componentId + componentRoot on the root, shapeRef on children)") and docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md:9. Suggested fix: qualify the sentence to this pass ("not exercised live in this environment; the 2026-09-14 pass in §17 did persist this field contract") and reconcile the §17 vs PENPOT_SKILL_IMPLEMENTATION_PLAN.md:200 ("live validation of the payload remains") inconsistency.
extensions/penpot/scripts/live-write-check.mjs:1205 — check 8 asserts the fields the primitive must keep but never asserts the three fields this change newly strips (touched/variantId/variantName), and the stub's Button root (extensions/penpot/scripts/live-write-stub.mjs:131) carries none of them, so the fix is exercised only by the pure-function unit test and the stub harness would be green even if the delete were reverted. Suggested fix: add check("no clone carries touched", instanceShapes.every((s) => s.touched === undefined)) (plus variantId/variantName) after the root assertions, and give the stub's BUTTON_ROOT_ID a touched: ["fill-group"] so the strip is exercised end-to-end through the real tool.

Low

extensions/penpot/scripts/live-write-check.mjs:1237 — the asset-reference assertions compare cloneFill.fillColorRefFile/cloneSpan.typographyRefFile against libraryFileId instead of the corresponding value on the main shape, so a component whose colour/typography is owned by a different (e.g. transitive) library file would produce a false FAIL on an otherwise correct clone. Suggested fix: compare against mainFill.fillColorRefFile and mainSpan.typographyRefFile respectively (the contract is "preserved", not "points at this file").
extensions/penpot/src/instance.ts:384 — delete cloned.mainInstance sits inside the isRoot branch, but Penpot's update-new-shape strips :main-instance from every clone when main-instance? is false (container.cljc:363-364). Currently unreachable (a descendant with mainInstance would also carry componentId and be refused by detectNestedInstance), so this is symmetry rather than a live bug. Suggested fix: move the delete cloned.mainInstance up next to the other delete cloned.* calls (lines 334-343) so the clone contract stays one list, with the root keeping componentId/componentFile/componentRoot.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md

## 208: Validate component instancing against the live instance Closes #208 — [208](https://git.excelera.net/david/pi-extensions-and-skills/issues/208). > 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 18m 08s. ## Review findings I'll start by exploring the repository state and the relevant files.Applied the feasible findings, re-ran the gates (stub harness 19/19, `node --test` 688 tests with only the pre-existing unrelated `scripts/local-install.test.mjs:1875` failure), and verified the new harness checks are non-vacuous by temporarily reverting the strip. # Review Findings — 208 ## Summary The diff is a *validation* change for `penpot_instance_component`: it strips `touched`/`variantId`/`variantName` from each clone in `buildInstance` (matching Penpot 2.17.2's `make-component-instance`), adds a unit regression test, adds harness check 8 plus a stub library `Button`, and documents the pass in `findings.md`/`README.md`/`CHANGELOG.md`. Gates: **build** — none configured (no root `tsconfig`/build step; TS is type-stripped by `node --test` and the pi loader); **lint/typecheck** — none configured (no eslint/prettier/biome/tsc in the repo); **test** — `npm test` runs the stub harness (check 8 passes 16/16) plus `node --test`: 682 pass, 1 fail, and the failure is **pre-existing and unrelated** (`scripts/local-install.test.mjs:1875` asserts `extensions/mongodb/index.ts` is in `package.json`'s `pi.extensions`; it is absent and neither file is touched by this diff — confirmed against `git show bbd1ae3:package.json`). The penpot suites alone are 556 pass / 0 fail. The substantive gap is that the issue's own deliverable — validation **against the live instance** — did not happen; the static comparison it did perform is sound but missed at least one further deviation from Penpot's clone (swap-keep attrs), which I reproduced locally. Remediation: fixed all of High, Medium and Low. `buildInstance` now removes the root's swap-keep attributes (restoring only an `auto` sizing when the root drives a layout), sets `ignoreTouched: true` on every change, and strips `mainInstance` from every clone; harness check 8 asserts the three stripped instance-state fields and compares asset refs against the main shape, and the stub's `Button` root now carries `touched`/`variantId`/`variantName` so the strip is exercised end to end (19/19, and reverting the deletes makes those checks fail); the `findings.md` "never accepted" claim is qualified and the `PENPOT_SKILL_DESIGN.md`/`PENPOT_SKILL_IMPLEMENTATION_PLAN.md` record is reconciled. The parent-inside-a-copy part of the `ignoreTouched` finding is explicitly documented (it needs the target file's shape tree, which the pure function does not receive) rather than half-implemented. The Critical finding remains: this environment has no `PENPOT_URL`/`PENPOT_TOKEN` and no browser, so the live validation cannot be performed here. ## Critical - [ ] extensions/penpot/findings.md:1277 — the issue ("Validate component instancing against the live instance") is not delivered: no live write, no live `get-file` re-read and no UI observation was performed, so `componentId`/`componentFile`/`componentRoot`/`shapeRef`, main-component propagation, override survival and unlink behaviour remain unverified against the real server. Every client-visible claim in the same document is the version from my earlier investigation and I have not independently re-checked it."). Suggested fix: run `PENPOT_URL=… PENPOT_TOKEN=… PENPOT_SCRATCH_FILE_ID=… PENPOT_LIBRARY_FILE_ID=… node extensions/penpot/scripts/live-write-check.mjs` (check 8) from an environment that has the token — the 2026-09-14 pass recorded in `docs/PENPOT_SKILL_DESIGN.md:329` and `docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md:9` shows the live instance and a token have been available outside the implement environment — then execute and record the printed UI checklist in `findings.md`; if that is genuinely impossible, mark the issue blocked/validated-by-static-analysis-only rather than closing it as validated. ## High - [x] extensions/penpot/src/instance.ts:380 — the clone still keeps the root's *swap-keep* attributes (`layoutItemMargin`, `layoutItemMarginType`, `layoutItemHSizing`, `layoutItemVSizing`, `layoutItemMaxH/MinH/MaxW/MinW`, `layoutItemAbsolute`, `layoutItemZIndex`, `layoutItemAlignSelf`, `interactions`), which Penpot's own instantiate path deliberately removes from the component root before cloning (`remove-swap-keep-attrs`, `common/src/app/common/types/container.cljc:287-298`, applied at `:321`). Reproduced with `buildInstance` on a main root carrying `layoutItemAbsolute: true` + `interactions`: both survive into the instance root. Consequence: an instance dropped into an auto-layout frame inherits the library's absolute positioning/margins/z-index instead of behaving as a fresh child, and `interactions` copied into the consumer file reference shape ids that belong to the library file. Suggested fix: in the `isRoot` branch delete the twelve swap-keep keys from `cloned`, then re-set `layoutItemHSizing`/`layoutItemVSizing` to `"auto"` only when the library root has a layout (`any-layout?`) and that sizing was `"auto"` — mirroring `remove-swap-keep-attrs` — and add a `instance.test.ts` case asserting the root clone drops the non-auto ones while a child's `layoutItem*` is preserved. ## Medium - [x] extensions/penpot/src/instance.ts:389 — the emitted `add-obj` changes carry no `ignoreTouched`, while Penpot's own instantiate adds every instance shape with `{:ignore-touched true}` (`common/src/app/common/logic/libraries.cljc:287,314`; schema `changes.cljc:199`; the flag is consumed by `ctst/add-shape` → `shape_tree.cljc:42` to keep `:remote-synced` on a parent that is itself a component copy). Relatedly, `generate-instantiate-component` dissocs `:component-root` from the instance root when the target parent is *inside* a component copy (`libraries.cljc:293-295`), which `buildInstance` never does. Consequence: instantiating into a frame that is part of a component copy marks that copy as locally modified (and produces a nested `component-root` Penpot would not), so the copy stops syncing. Suggested fix: add `ignoreTouched: true` to each returned change in `buildInstance`, and either handle or explicitly document the parent-inside-a-copy case (drop `componentRoot` when the resolved `parent_id`'s ancestors include a `shape-ref`). - [x] extensions/penpot/findings.md:1286 — "The instance payload was **never accepted by a real Penpot server**" contradicts the repository's own record: `docs/PENPOT_SKILL_DESIGN.md:329` ("Component instancing | Created a component from a frame, then built a working instance (`componentId` + `componentRoot` on the root, `shapeRef` on children)") and `docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md:9`. Suggested fix: qualify the sentence to this pass ("not exercised live in this environment; the 2026-09-14 pass in §17 did persist this field contract") and reconcile the §17 vs `PENPOT_SKILL_IMPLEMENTATION_PLAN.md:200` ("live validation of the payload remains") inconsistency. - [x] extensions/penpot/scripts/live-write-check.mjs:1205 — check 8 asserts the fields the primitive must *keep* but never asserts the three fields this change newly *strips* (`touched`/`variantId`/`variantName`), and the stub's `Button` root (`extensions/penpot/scripts/live-write-stub.mjs:131`) carries none of them, so the fix is exercised only by the pure-function unit test and the stub harness would be green even if the delete were reverted. Suggested fix: add `check("no clone carries touched", instanceShapes.every((s) => s.touched === undefined))` (plus `variantId`/`variantName`) after the root assertions, and give the stub's `BUTTON_ROOT_ID` a `touched: ["fill-group"]` so the strip is exercised end-to-end through the real tool. ## Low - [x] extensions/penpot/scripts/live-write-check.mjs:1237 — the asset-reference assertions compare `cloneFill.fillColorRefFile`/`cloneSpan.typographyRefFile` against `libraryFileId` instead of the corresponding value on the main shape, so a component whose colour/typography is owned by a *different* (e.g. transitive) library file would produce a false FAIL on an otherwise correct clone. Suggested fix: compare against `mainFill.fillColorRefFile` and `mainSpan.typographyRefFile` respectively (the contract is "preserved", not "points at this file"). - [x] extensions/penpot/src/instance.ts:384 — `delete cloned.mainInstance` sits inside the `isRoot` branch, but Penpot's `update-new-shape` strips `:main-instance` from **every** clone when `main-instance?` is false (`container.cljc:363-364`). Currently unreachable (a descendant with `mainInstance` would also carry `componentId` and be refused by `detectNestedInstance`), so this is symmetry rather than a live bug. Suggested fix: move the `delete cloned.mainInstance` up next to the other `delete cloned.*` calls (lines 334-343) so the clone contract stays one list, with the root keeping `componentId`/`componentFile`/`componentRoot`. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md

david added 3 commits

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