issue-198: Implement penpot_add_frame with auto-layout props and absolute-positioning fallback #231

Merged

david merged 2 commits from feature/issue-198/implement-penpot-add-frame-with-auto-layout-props into main

2026-09-15 03:12:18 +00:00

david commented

2026-09-15 03:12:17 +00:00

Owner

198: Implement penpot_add_frame with auto-layout props and absolute-positioning fallback

Closes #198 — 198.

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 52s.

Review findings

I'll start by exploring the repo structure and reading the relevant files.Now let me examine the test files and remaining sources.Now I'll implement the fixes. Starting with the High severity commit verification fallback:Now the Low severity fix in shapes.ts — deriving the union types from exported tuples:Now frame.ts — schema enum derivation, minLength, fill-ref handling, and parent validation:Now the parent validation and fill handling in frame.ts:Now add findShapeOnPage to fileShape.ts:All gates pass except the pre-existing manifest failure. Let me confirm the final state of the diff before reporting.# Review Findings — 198

Summary

All five findings are resolved in one pass. The High finding restored the fail-closed default branch in verifyStagedChanges (plus the describeUnverifiableChange fallback and two tests covering an unknown change type with and without identifying fields). Both Medium findings are fixed: name now uses the string keyword minLength: 1 (with a schema assertion in the existing declaration test), and parent_id is resolved against the target page's object map via a new pure findShapeOnPage helper in fileShape.ts, so a missing or non-frame parent is refused before anything is staged (tests for unknown, non-frame, valid-frame and explicit-root parents). Both Low findings are fixed: fill_color_ref without fill_color is now rejected instead of dropped, a fill_color_ref_file parameter attributes a linked-library asset to its owning file (defaulting to file_id), and the five layout enums are now built from exported literal tuples in shapes.ts that the payload types are themselves derived from, so schema and type cannot drift. Gates: npm test (the only script; there is no build or lint script) runs 403/404, the single failure being the pre-existing scripts/local-install.test.mjs:1875 mongodb-manifest assertion, which is unchanged from main; the full penpot suite is 277/277. I also verified with a tsc --strict run (TypeScript installed outside the repo, since none is a project dependency) that the set of type errors is byte-for-byte identical to HEAD, i.e. the type-level enum change introduced no new errors.

Critical

(none)

High

extensions/penpot/src/commit.ts:126 — the switch (change.type) in verifyStagedChanges no longer has a default branch, so a staged change whose type has no verification case is silently skipped and the function returns verified: true with no evidence (previously the removed describeUnverifiableChange fallback pushed such a change into missing). The docstring at commit.ts:80-82 still promises the old fail-closed behaviour, and the next milestones (add-rect/text/instance) will add change types to this union, so a batch containing an unverified type could be reported verified and its changeset cleared after a write that may not have applied. Suggested fix: restore the fallback, e.g. default: { const raw = change as { type: ChangeType; id?: string; name?: string }; missing.push({ type: raw.type, id: raw.id ?? "(unknown id)", name: raw.name ?? "(unknown name)" }); } (or an assertNever that reports the unknown type as missing), and add a test staging an unknown change type and asserting verified: false.

Medium

extensions/penpot/src/tools/frame.ts:134 — name: Type.String({ minimum: 1, … }) uses the numeric minimum keyword, which JSON Schema/TypeBox ignores for strings, so an empty layer name passes schema validation (confirmed: Value.Check(Type.String({ minimum: 1 }), "") returns true; the built schema is {"type":"string","minimum":1}). The intent is clearly a non-empty name. Suggested fix: use minLength: 1 (verified to reject "") and add a schema assertion for it in the add_frame declares the required … test.
extensions/penpot/src/tools/frame.ts:317 — parent_id is copied verbatim into both the shape's frameId and parentId without checking that it exists or is a frame. A non-frame id (rect/group, discoverable via penpot_get_file) produces a shape whose frameId is not a frame id — at best a server rejection and at worst a wrongly-parented shape. Suggested fix: when parent_id is supplied, resolve it from the selected page's object map (read the page objects, or use a targeted get-file/get-file-fragment read) and fail with a clear error before staging if it is absent or type !== "frame"; add a test for an unknown/non-frame parent id.

Low

extensions/penpot/src/tools/frame.ts:264 — fill_color_ref is silently dropped when fill_color is omitted (the if (params.fill_color === undefined) return [] at line 257 short-circuits it), and fillColorRefFile is hard-coded to params.file_id. Since penpot_list_library can resolve colour ids from linked library files, a reference to an asset owned by another file would be attributed to the target file. Suggested fix: reject fill_color_ref without fill_color (or warn), and add an optional fill_color_ref_file parameter defaulting to file_id so a linked-library asset can be referenced correctly.
extensions/penpot/src/tools/frame.ts:44 — the layout enums are re-declared as independent TypeBox unions (layoutType, flexDirection, alignItems, justifyContent, wrapType) rather than being tied to the exported LayoutType/FlexDirection/AlignItems/JustifyContent/WrapType types in shapes.ts, so the schema and the payload type can drift apart silently. Suggested fix: derive the literals from the shared unions (e.g. Type.Union<LayoutType>([Type.Literal("flex"), Type.Literal("grid")]) — or a satisfies/compile-time assertion over the literal tuples) so adding a value in one place cannot diverge from the other.

## 198: Implement penpot_add_frame with auto-layout props and absolute-positioning fallback Closes #198 — [198](https://git.excelera.net/david/pi-extensions-and-skills/issues/198). > 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 52s. ## Review findings I'll start by exploring the repo structure and reading the relevant files.Now let me examine the test files and remaining sources.Now I'll implement the fixes. Starting with the High severity commit verification fallback:Now the Low severity fix in `shapes.ts` — deriving the union types from exported tuples:Now `frame.ts` — schema enum derivation, `minLength`, fill-ref handling, and parent validation:Now the parent validation and fill handling in `frame.ts`:Now add `findShapeOnPage` to `fileShape.ts`:All gates pass except the pre-existing manifest failure. Let me confirm the final state of the diff before reporting.# Review Findings — 198 ## Summary All five findings are resolved in one pass. The High finding restored the fail-closed `default` branch in `verifyStagedChanges` (plus the `describeUnverifiableChange` fallback and two tests covering an unknown change type with and without identifying fields). Both Medium findings are fixed: `name` now uses the string keyword `minLength: 1` (with a schema assertion in the existing declaration test), and `parent_id` is resolved against the target page's object map via a new pure `findShapeOnPage` helper in `fileShape.ts`, so a missing or non-frame parent is refused before anything is staged (tests for unknown, non-frame, valid-frame and explicit-root parents). Both Low findings are fixed: `fill_color_ref` without `fill_color` is now rejected instead of dropped, a `fill_color_ref_file` parameter attributes a linked-library asset to its owning file (defaulting to `file_id`), and the five layout enums are now built from exported literal tuples in `shapes.ts` that the payload types are themselves derived from, so schema and type cannot drift. Gates: `npm test` (the only script; there is no build or lint script) runs 403/404, the single failure being the pre-existing `scripts/local-install.test.mjs:1875` mongodb-manifest assertion, which is unchanged from `main`; the full penpot suite is 277/277. I also verified with a `tsc --strict` run (TypeScript installed outside the repo, since none is a project dependency) that the set of type errors is byte-for-byte identical to `HEAD`, i.e. the type-level enum change introduced no new errors. ## Critical - [ ] *(none)* ## High - [x] extensions/penpot/src/commit.ts:126 — the `switch (change.type)` in `verifyStagedChanges` no longer has a `default` branch, so a staged change whose type has no verification case is silently skipped and the function returns `verified: true` with no evidence (previously the removed `describeUnverifiableChange` fallback pushed such a change into `missing`). The docstring at commit.ts:80-82 still promises the old fail-closed behaviour, and the next milestones (add-rect/text/instance) will add change types to this union, so a batch containing an unverified type could be reported verified and its changeset cleared after a write that may not have applied. Suggested fix: restore the fallback, e.g. `default: { const raw = change as { type: ChangeType; id?: string; name?: string }; missing.push({ type: raw.type, id: raw.id ?? "(unknown id)", name: raw.name ?? "(unknown name)" }); }` (or an `assertNever` that reports the unknown type as missing), and add a test staging an unknown change type and asserting `verified: false`. ## Medium - [x] extensions/penpot/src/tools/frame.ts:134 — `name: Type.String({ minimum: 1, … })` uses the numeric `minimum` keyword, which JSON Schema/TypeBox ignores for strings, so an empty layer name passes schema validation (confirmed: `Value.Check(Type.String({ minimum: 1 }), "")` returns `true`; the built schema is `{"type":"string","minimum":1}`). The intent is clearly a non-empty name. Suggested fix: use `minLength: 1` (verified to reject `""`) and add a schema assertion for it in the `add_frame declares the required …` test. - [x] extensions/penpot/src/tools/frame.ts:317 — `parent_id` is copied verbatim into both the shape's `frameId` and `parentId` without checking that it exists or is a frame. A non-frame id (rect/group, discoverable via `penpot_get_file`) produces a shape whose `frameId` is not a frame id — at best a server rejection and at worst a wrongly-parented shape. Suggested fix: when `parent_id` is supplied, resolve it from the selected page's object map (read the page objects, or use a targeted `get-file`/`get-file-fragment` read) and fail with a clear error before staging if it is absent or `type !== "frame"`; add a test for an unknown/non-frame parent id. ## Low - [x] extensions/penpot/src/tools/frame.ts:264 — `fill_color_ref` is silently dropped when `fill_color` is omitted (the `if (params.fill_color === undefined) return []` at line 257 short-circuits it), and `fillColorRefFile` is hard-coded to `params.file_id`. Since `penpot_list_library` can resolve colour ids from linked library files, a reference to an asset owned by another file would be attributed to the target file. Suggested fix: reject `fill_color_ref` without `fill_color` (or warn), and add an optional `fill_color_ref_file` parameter defaulting to `file_id` so a linked-library asset can be referenced correctly. - [x] extensions/penpot/src/tools/frame.ts:44 — the layout enums are re-declared as independent TypeBox unions (`layoutType`, `flexDirection`, `alignItems`, `justifyContent`, `wrapType`) rather than being tied to the exported `LayoutType`/`FlexDirection`/`AlignItems`/`JustifyContent`/`WrapType` types in `shapes.ts`, so the schema and the payload type can drift apart silently. Suggested fix: derive the literals from the shared unions (e.g. `Type.Union<LayoutType>([Type.Literal("flex"), Type.Literal("grid")])` — or a `satisfies`/compile-time assertion over the literal tuples) so adding a value in one place cannot diverge from the other.

david added 2 commits

2026-09-15 03:12:17 +00:00