From 9f598fca5b55ed67a2a70902210e1c5927fbbcbd Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Wed, 14 Oct 2026 15:26:24 +0200 Subject: [PATCH 1/2] :sparkles: Add component management to plugins API (#12303) --- .../app/main/data/workspace/libraries.cljs | 3 +- frontend/src/app/plugins/library.cljs | 90 ++++++- frontend/src/app/plugins/shape.cljs | 32 +++ .../frontend_tests/plugins/library_test.cljs | 161 ++++++++++++ plugins/CHANGELOG.md | 3 + .../src/generated/api-surface.json | 126 ++++++++++ .../src/tests/components.test.ts | 234 ++++++++++++++++++ plugins/libs/plugin-types/index.d.ts | 48 ++++ 8 files changed, 691 insertions(+), 6 deletions(-) diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index 2113f76ea1..b84faf4356 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -531,7 +531,8 @@ (-> (pcb/empty-changes it nil) (cll/generate-duplicate-component library component-id new-component-id))] (rx/of - (ptk/data-event :layout/update {:ids [(:id main-instance)]}) + (ptk/data-event :layout/update {:page-id (:main-instance-page (get-in library [:data :components component-id])) + :ids [(:id main-instance)]}) (dch/commit-changes changes))))))) (defn delete-component diff --git a/frontend/src/app/plugins/library.cljs b/frontend/src/app/plugins/library.cljs index 56782eb1e2..a5a70e1cfa 100644 --- a/frontend/src/app/plugins/library.cljs +++ b/frontend/src/app/plugins/library.cljs @@ -19,6 +19,7 @@ [app.common.types.variant :as ctv] [app.common.uuid :as uuid] [app.main.data.plugins :as dp] + [app.main.data.workspace :as dw] [app.main.data.workspace.libraries :as dwl] [app.main.data.workspace.texts :as dwt] [app.main.data.workspace.variants :as dwv] @@ -845,12 +846,81 @@ :remove (fn [] - (cond - (not (r/check-permission plugin-id "library:write")) - (u/not-valid plugin-id :remove "Plugin doesn't have 'library:write' permission") + (let [component (u/locate-library-component file-id id)] + (cond + (not (r/check-permission plugin-id "library:write")) + (u/not-valid plugin-id :remove "Plugin doesn't have 'library:write' permission") - :else - (st/emit! (dwl/delete-component {:id id})))) + (not= file-id (:current-file-id @st/state)) + (u/not-valid plugin-id :remove "Can only remove components from the current file's library") + + (nil? component) + (u/not-valid plugin-id :remove "Component does not exist") + + (not (:deleted component)) + (st/emit! (-> (dwl/delete-component {:id id}) + (se/add-event plugin-id)))))) + + :annotation + {:get (fn [] (:annotation (u/locate-library-component file-id id))) + :set + (fn [value] + (let [component (u/locate-library-component file-id id)] + (cond + (not (r/check-permission plugin-id "library:write")) + (u/not-valid plugin-id :annotation "Plugin doesn't have 'library:write' permission") + + (not= file-id (:current-file-id @st/state)) + (u/not-valid plugin-id :annotation "Can only edit annotations in the current file's library") + + (nil? component) + (u/not-valid plugin-id :annotation "Component does not exist") + + (:deleted component) + (u/not-valid plugin-id :annotation "Cannot edit a deleted component") + + (not (or (nil? value) (and (string? value) (<= (count value) 300)))) + (u/not-valid plugin-id :annotation "Expected null or a string of at most 300 characters") + + :else + (st/emit! (-> (dw/update-component-annotation id value) + (se/add-event plugin-id))))))} + + :duplicate + (fn [] + (let [component (u/locate-library-component file-id id)] + (cond + (not (r/check-permission plugin-id "library:write")) + (u/not-valid plugin-id :duplicate "Plugin doesn't have 'library:write' permission") + + (not= file-id (:current-file-id @st/state)) + (u/not-valid plugin-id :duplicate "Can only duplicate components in the current file's library") + + (nil? component) + (u/not-valid plugin-id :duplicate "Component does not exist") + + (:deleted component) + (u/not-valid plugin-id :duplicate "Cannot duplicate a deleted component") + + :else + (let [new-id (uuid/next)] + (st/emit! (-> (dwl/duplicate-component file-id id new-id) + (se/add-event plugin-id))) + (lib-component-proxy plugin-id file-id new-id))))) + + :restore + (fn [] + (let [component (u/locate-library-component file-id id)] + (cond + (not (r/check-permission plugin-id "library:write")) + (u/not-valid plugin-id :restore "Plugin doesn't have 'library:write' permission") + + (nil? component) + (u/not-valid plugin-id :restore "Component does not exist") + + (:deleted component) + (st/emit! (-> (dwl/restore-component file-id id) + (se/add-event plugin-id)))))) :instance (fn [] @@ -1080,6 +1150,16 @@ (map #(lib-component-proxy plugin-id file-id %)))] (apply array components)))} + :deletedComponents + {:get + (fn [] + (let [components (->> (u/locate-file file-id) + :data + :components + (filter (comp :deleted second)) + (map #(lib-component-proxy plugin-id file-id (first %))))] + (apply array components)))} + :tokens {:this true :get diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 6be18bd4fa..b34b329598 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -1587,6 +1587,38 @@ target-id true))))) + :updateMain + (fn [] + (let [shape (u/locate-shape file-id page-id id) + objects (u/locate-objects file-id page-id) + component (when (:component-id shape) + (u/locate-library-component (:component-file shape) (:component-id shape)))] + (cond + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :updateMain "Plugin doesn't have 'content:write' permission") + + (not (r/check-permission plugin-id "library:write")) + (u/not-valid plugin-id :updateMain "Plugin doesn't have 'library:write' permission") + + (not= file-id (:current-file-id @st/state)) + (u/not-valid plugin-id :updateMain "Can only update a component from a copy in the current file") + + (not (u/page-active? page-id)) + (u/not-valid plugin-id :updateMain "Cannot modify a page that is not currently active") + + (not (and (ctk/instance-head? shape) (ctk/in-component-copy? shape))) + (u/not-valid plugin-id :updateMain "The shape is not a component copy head") + + (u/inside-component-copy? objects shape) + (u/not-valid plugin-id :updateMain "Cannot update the main from a shape inside another component copy") + + (or (nil? component) (:deleted component)) + (u/not-valid plugin-id :updateMain "The main component does not exist or is deleted") + + :else + (st/emit! (-> (dwl/update-component-sync id (:component-file shape)) + (se/add-event plugin-id)))))) + :resetOverrides (fn [] (let [shape (u/locate-shape file-id page-id id)] diff --git a/frontend/test/frontend_tests/plugins/library_test.cljs b/frontend/test/frontend_tests/plugins/library_test.cljs index 70d3e3fb7d..e7e9efb646 100644 --- a/frontend/test/frontend_tests/plugins/library_test.cljs +++ b/frontend/test/frontend_tests/plugins/library_test.cljs @@ -6,6 +6,12 @@ (ns frontend-tests.plugins.library-test (:require + [app.common.files.changes-builder :as pcb] + [app.common.logic.shapes :as cls] + [app.common.test-helpers.components :as cthc] + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as thi] + [app.common.test-helpers.shapes :as cths] [app.common.types.component :as ctk] [app.common.uuid :as uuid] [app.main.data.event :as ev] @@ -14,8 +20,10 @@ [app.main.data.workspace.variants :as dwv] [app.main.repo :as rp] [app.main.store :as st] + [app.main.worker :as mw] [app.plugins.library :as library] [app.plugins.register :as r] + [app.plugins.shape :as shape] [app.plugins.text :as text] [app.plugins.utils :as u] [app.util.object :as obj] @@ -24,10 +32,163 @@ [cuerdas.core :as str] [frontend-tests.helpers.async :as async] [frontend-tests.helpers.mock :as mock] + [frontend-tests.helpers.state :as ths] [potok.v2.core :as ptk])) (def ^:private plugin-id "00000000-0000-0000-0000-000000000000") +(defn- file-with-component + [] + (-> (cthf/sample-file :restore-file :page-label :main-page) + (cths/add-sample-shape :main :type :frame) + (cths/add-sample-shape :child :parent-label :main) + (cthc/make-component :restorable :main) + (cthc/instantiate-component :restorable :copy))) + +(defn- file-with-deleted-component + [] + (let [file (file-with-component) + page (cthf/current-page file) + changes (-> (pcb/empty-changes nil (:id page)) + (cls/generate-delete-shapes (:data file) page (:objects page) #{(thi/id :main)} {}) + second)] + (cthf/apply-changes file changes))) + +(t/deftest ^:async component-restore-preserves-the-users-view + (doseq [scenario [:same-page :other-page :linked-library]] + (let [file (cond-> (file-with-deleted-component) + (= scenario :other-page) (cthf/add-sample-page :other-page)) + store (if (= scenario :linked-library) + (ths/setup-store (cthf/sample-file :local-file) + {:renderer :svg :libraries [file]}) + (ths/setup-store file {:renderer :svg})) + view {:selected #{(thi/id :copy)} + :zoom 2 + :vport {:x 100 :y 200 :width 800 :height 600} + :vbox {:x 50 :y 100 :width 400 :height 300}} + view-of #(select-keys % [:current-file-id :current-page-id :workspace-local])] + (ptk/emit! store #(assoc % :workspace-local view)) + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (let [before (view-of @store)] + (await + (mock/with-mocks* + {st/state store + st/emit! (mock/stub (fn [& events] (apply ptk/emit! store events))) + mw/ask! (mock/stub (fn [_] (rx/empty))) + shape/lib-component-proxy library/lib-component-proxy} + (let [copy (shape/shape-proxy plugin-id (:id file) (thi/id :main-page) (thi/id :copy)) + component (.component copy)] + (t/is (true? (get-in @store [:files (:id file) :data :components (thi/id :restorable) :deleted]))) + (.restore component) + (await (async/wait-for #(not (get-in @store [:files (:id file) :data :components (thi/id :restorable) :deleted])) + "component restored")) + (.restore component) + (await (async/settle)) + (t/is (= (thi/id :main) + (get-in @store [:files (:id file) :data :pages-index (thi/id :main-page) :objects (thi/id :main) :id]))) + (t/is (= (thi/id :child) + (get-in @store [:files (:id file) :data :pages-index (thi/id :main-page) :objects (thi/id :child) :id]))) + (t/is (= before (view-of @store))) + (t/is (= [(str (thi/id :restorable))] + (mapv #(.-id %) (.-components (library/library-proxy plugin-id (:id file))))))))))))) + +(t/deftest ^:async component-restore-checks-library-write-permission + (let [file (file-with-deleted-component) + store (ths/setup-store file {:renderer :svg}) + events (atom [])] + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (await + (mock/with-mocks* + {st/state store + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + r/check-permission (mock/stub (constantly false))} + (let [component (library/lib-component-proxy plugin-id (:id file) (thi/id :restorable))] + (t/is (thrown-with-msg? js/Error #"library:write" (.restore component))) + (await (async/settle)) + (t/is (empty? @events)) + (t/is (true? (get-in @store [:files (:id file) :data :components (thi/id :restorable) :deleted])))))))) + +(t/deftest ^:async component-restore-rejects-a-missing-component + (let [file (cthf/sample-file :restore-file) + store (ths/setup-store file {:renderer :svg})] + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (await + (mock/with-mocks* + {st/state store} + (let [component (library/lib-component-proxy plugin-id (:id file) (uuid/next))] + (t/is (thrown-with-msg? js/Error #"Component does not exist" (.restore component)))))))) + +(def ^:private component-mutations + [["remove" #(.remove %)] + ["duplicate" #(.duplicate %)] + ["annotation" #(set! (.-annotation %) "Usage")]]) + +(t/deftest ^:async component-mutations-check-library-write-permission + (let [file (file-with-component) + store (ths/setup-store file {:renderer :svg}) + before @store + events (atom [])] + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (await + (mock/with-mocks* + {st/state store + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + r/check-permission (mock/stub (constantly false))} + (let [component (library/lib-component-proxy plugin-id (:id file) (thi/id :restorable))] + (doseq [[operation mutate] component-mutations] + (t/testing operation + (t/is (thrown-with-msg? js/Error #"library:write" (mutate component))))) + (await (async/settle)) + (t/is (empty? @events)) + (t/is (= (:files before) (:files @store)))))))) + +(t/deftest ^:async component-mutations-reject-linked-libraries + (let [library (file-with-component) + file (cthf/sample-file :consumer-file) + store (ths/setup-store file {:renderer :svg :libraries [library]}) + before @store + events (atom [])] + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (await + (mock/with-mocks* + {st/state store + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} + (let [component (library/lib-component-proxy plugin-id (:id library) (thi/id :restorable))] + (doseq [[operation mutate] component-mutations] + (t/testing operation + (t/is (thrown-with-msg? js/Error #"current file" (mutate component))))) + (await (async/settle)) + (t/is (empty? @events)) + (t/is (= (:files before) (:files @store)))))))) + +(t/deftest ^:async component-mutations-reject-missing-components + (let [file (cthf/sample-file :restore-file) + store (ths/setup-store file {:renderer :svg})] + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (await + (mock/with-mocks* + {st/state store} + (let [component (library/lib-component-proxy plugin-id (:id file) (uuid/next))] + (doseq [[operation mutate] component-mutations] + (t/testing operation + (t/is (thrown-with-msg? js/Error #"Component does not exist" (mutate component)))))))))) + +(t/deftest ^:async update-main-checks-content-and-library-write-permissions + (doseq [denied ["content:write" "library:write"]] + (let [file (file-with-component) + store (ths/setup-store file {:renderer :svg}) + events (atom [])] + (ptk/emit! store #(assoc-in % [:plugins :flags plugin-id :throw-validation-errors] true)) + (await + (mock/with-mocks* + {st/state store + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + r/check-permission (mock/stub (fn [_ permission] (not= permission denied)))} + (let [copy (shape/shape-proxy plugin-id (:id file) (thi/id :main-page) (thi/id :copy))] + (t/is (thrown-with-msg? js/Error (re-pattern denied) (.updateMain copy))) + (await (async/settle)) + (t/is (empty? @events)))))))) + (t/deftest library-asset-proxies-expose-library-id (let [file-id (random-uuid) id (random-uuid)] diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 4321ec2fc9..af3cc43b0b 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -2,6 +2,8 @@ ### 🚀 Features +- **plugin-types:** Added `LibraryComponent.restore()` to restore a deleted component and its main instance without changing the current page, selection, zoom, or viewport. Requires `library:write` permission. +- **plugin-types:** Added `Shape.updateMain()`, `LibraryComponent.duplicate()`, writable component `annotation`, and `Library.deletedComponents`. Declared the existing `LibraryComponent.remove()` method. - **plugin-types:** Added `paddingType` (`'simple' | 'multiple'`) to flex and grid layouts and `marginType` (`'simple' | 'multiple'`) to layout children, exposing whether the four padding/margin sides are mirrored or honoured independently. - **plugin-types**: Added `waitForLayoutUpdate` to wait until pending layout updates have finished. It rejects when the optional timeout elapses, defaulting to 30 seconds so a wait never hangs. - **plugin-types**: Added `waitForLayoutUpdate` to the `Shape` interface to wait until the pending layout updates of a shape and its children have finished @@ -9,6 +11,7 @@ ### 🩹 Fixes +- **plugins-runtime:** `LibraryComponent.remove()` now rejects linked-library and missing component handles, and does nothing for an already deleted component. - **plugins-runtime**: An interaction obtained from `Shape.interactions` now keeps addressing that interaction instead of the position it held when the array was read. Removing every interaction of a shape from a single read removes all of them rather than leaving some behind, and writing through a held interaction after an earlier one is removed no longer lands on a different interaction. - **plugins-runtime**: `Shape.removeInteraction()` now rejects an interaction belonging to a different shape with a validation error, instead of removing whichever interaction sat at the same position on the target shape. - **plugins-runtime**: Writing `trigger`, `delay` or `action` on an interaction the shape no longer has now raises a validation error. The write used to be sent to the workspace with no position to apply it at, where it failed out of the plugin's reach: nothing was written and nothing was reported. diff --git a/plugins/apps/plugin-api-test-suite/src/generated/api-surface.json b/plugins/apps/plugin-api-test-suite/src/generated/api-surface.json index b32168ec1b..5034ba9730 100644 --- a/plugins/apps/plugin-api-test-suite/src/generated/api-surface.json +++ b/plugins/apps/plugin-api-test-suite/src/generated/api-surface.json @@ -273,6 +273,7 @@ "createColor", "createComponent", "createTypography", + "deletedComponents", "id", "name", "tokens", @@ -287,9 +288,13 @@ "opacity" ], "LibraryComponent": [ + "annotation", + "duplicate", "instance", "isVariant", "mainInstance", + "remove", + "restore", "transformInVariant" ], "LibraryContext": [ @@ -454,6 +459,7 @@ "swapComponent", "switchVariant", "tokens", + "updateMain", "visible", "waitForLayoutUpdate", "width", @@ -1124,6 +1130,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -1630,6 +1642,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -2774,6 +2792,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -4022,6 +4046,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -4612,6 +4642,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -4994,6 +5030,12 @@ "type": "LibraryComponent", "array": true }, + "deletedComponents": { + "decl": "Library", + "kind": "get", + "type": "LibraryComponent", + "array": true + }, "tokens": { "decl": "Library", "kind": "get", @@ -5154,6 +5196,30 @@ } }, "LibraryComponent": { + "annotation": { + "decl": "LibraryComponent", + "kind": "getset", + "type": null, + "array": false + }, + "duplicate": { + "decl": "LibraryComponent", + "kind": "method", + "type": "LibraryComponent", + "array": false + }, + "remove": { + "decl": "LibraryComponent", + "kind": "method", + "type": null, + "array": false + }, + "restore": { + "decl": "LibraryComponent", + "kind": "method", + "type": null, + "array": false + }, "instance": { "decl": "LibraryComponent", "kind": "method", @@ -5524,6 +5590,30 @@ "type": null, "array": false }, + "annotation": { + "decl": "LibraryComponent", + "kind": "getset", + "type": null, + "array": false + }, + "duplicate": { + "decl": "LibraryComponent", + "kind": "method", + "type": "LibraryComponent", + "array": false + }, + "remove": { + "decl": "LibraryComponent", + "kind": "method", + "type": null, + "array": false + }, + "restore": { + "decl": "LibraryComponent", + "kind": "method", + "type": null, + "array": false + }, "instance": { "decl": "LibraryComponent", "kind": "method", @@ -6246,6 +6336,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -7088,6 +7184,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -7622,6 +7724,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -8222,6 +8330,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -8794,6 +8908,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", @@ -11098,6 +11218,12 @@ "type": null, "array": false }, + "updateMain": { + "decl": "ShapeBase", + "kind": "method", + "type": null, + "array": false + }, "switchVariant": { "decl": "ShapeBase", "kind": "method", diff --git a/plugins/apps/plugin-api-test-suite/src/tests/components.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/components.test.ts index ecdd6c5118..92d68fd6aa 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/components.test.ts +++ b/plugins/apps/plugin-api-test-suite/src/tests/components.test.ts @@ -199,6 +199,66 @@ describe('Component instances', () => { } }); + test('restore restores a deleted component without changing the view', async (ctx) => { + const originalPage = ctx.penpot.currentPage; + expect(originalPage).not.toBeNull(); + if (!originalPage) return; + + const comp = makeComponent(ctx); + const main = comp.mainInstance() as Board; + const mainId = main.id; + const childIds = main.children.map((child) => child.id); + const inst = comp.instance(); + ctx.board.appendChild(inst); + comp.remove(); + await ctx.penpot.waitForLayoutUpdate(); + expect( + ctx.penpot.library.local.components.some((c) => c.id === comp.id), + ).toBe(false); + expect(originalPage.getShapeById(mainId)).toBeNull(); + + const otherPage = ctx.penpot.createPage(); + try { + await ctx.penpot.openPage(otherPage); + const selected = ctx.penpot.createRectangle(); + (otherPage.root as Board).appendChild(selected); + ctx.penpot.selection = [selected]; + ctx.penpot.viewport.zoom = 2; + ctx.penpot.viewport.center = { x: 300, y: 200 }; + await ctx.penpot.waitForLayoutUpdate(); + + const zoom = ctx.penpot.viewport.zoom; + const center = ctx.penpot.viewport.center; + const deleted = inst.component(); + expect(deleted).not.toBeNull(); + if (!deleted) return; + + deleted.restore(); + await waitFor(() => + ctx.penpot.library.local.components.some((c) => c.id === comp.id), + ); + await ctx.penpot.waitForLayoutUpdate(); + + expect( + ctx.penpot.library.local.components.some((c) => c.id === comp.id), + ).toBe(true); + expect(originalPage.getShapeById(mainId)?.id).toBe(mainId); + expect( + (deleted.mainInstance() as Board).children.map((child) => child.id), + ).toEqual(childIds); + expect(ctx.penpot.currentPage?.id).toBe(otherPage.id); + expect(ctx.penpot.selection.map((shape) => shape.id)).toEqual([ + selected.id, + ]); + expect(ctx.penpot.viewport.zoom).toBeCloseTo(zoom); + expect(ctx.penpot.viewport.center.x).toBeCloseTo(center.x); + expect(ctx.penpot.viewport.center.y).toBeCloseTo(center.y); + } finally { + await ctx.penpot.openPage(originalPage); + otherPage.remove(); + } + }); + test('detach turns an instance into a basic shape', (ctx) => { const inst = instanceOf(ctx); inst.detach(); @@ -319,3 +379,177 @@ describe('Component instances', () => { expect(inst.isComponentInstance()).toBeFalsy(); }); }); + +describe('Component assets', () => { + test('duplicate creates an independent component with its annotation', async (ctx) => { + const source = makeComponent(ctx); + source.annotation = 'Button usage'; + const original = source.mainInstance() as Board; + const duplicate = source.duplicate(); + await ctx.penpot.waitForLayoutUpdate(); + const main = duplicate.mainInstance() as Board; + ctx.board.appendChild(main); + + expect(duplicate.id).not.toBe(source.id); + expect(main.id).not.toBe(original.id); + expect(main.isComponentMainInstance()).toBe(true); + expect(main.component()?.id).toBe(duplicate.id); + expect(main.children).toHaveLength(original.children.length); + expect(main.children[0].id).not.toBe(original.children[0].id); + expect(duplicate.annotation).toBe('Button usage'); + duplicate.name = 'Duplicate button'; + expect(source.name).not.toBe(duplicate.name); + }); + + test('annotations can be created, edited, and removed', async (ctx) => { + const comp = makeComponent(ctx); + expect(comp.annotation).toBeNull(); + comp.annotation = 'Use for primary actions'; + expect(comp.annotation).toBe('Use for primary actions'); + comp.annotation = 'Use for secondary actions'; + expect(comp.annotation).toBe('Use for secondary actions'); + expect(() => { + comp.annotation = 42 as unknown as string; + }).toThrow(); + expect(() => { + comp.annotation = 'x'.repeat(301); + }).toThrow(); + expect(comp.annotation).toBe('Use for secondary actions'); + comp.annotation = null; + await ctx.penpot.waitForLayoutUpdate(); + expect(comp.annotation).toBeNull(); + }); + + test('deletedComponents lists and restores a component without surviving copies', async (ctx) => { + const comp = makeComponent(ctx); + const mainId = comp.mainInstance().id; + const page = ctx.penpot.currentPage; + comp.remove(); + await ctx.penpot.waitForLayoutUpdate(); + expect(page?.getShapeById(mainId)).toBeNull(); + expect( + ctx.penpot.library.local.components.some((c) => c.id === comp.id), + ).toBe(false); + + const deleted = ctx.penpot.library.local.deletedComponents.find( + (c) => c.id === comp.id, + ); + expect(deleted).toBeDefined(); + if (!deleted) return; + deleted.remove(); + deleted.restore(); + await ctx.penpot.waitForLayoutUpdate(); + expect(page?.getShapeById(mainId)?.id).toBe(mainId); + expect( + ctx.penpot.library.local.components.some((c) => c.id === comp.id), + ).toBe(true); + expect( + ctx.penpot.library.local.deletedComponents.some((c) => c.id === comp.id), + ).toBe(false); + }); + + test('updateMain applies a copy override to the main and other copies', async (ctx) => { + const childColor = (board: Board) => { + const fill = board.children[0].fills[0]; + return typeof fill === 'string' ? fill : fill?.fillColor; + }; + const comp = makeComponent(ctx); + const main = comp.mainInstance() as Board; + const copy = comp.instance() as Board; + const other = comp.instance() as Board; + ctx.board.appendChild(copy); + ctx.board.appendChild(other); + copy.children[0].fills = [{ fillColor: '#ff0000', fillOpacity: 1 }]; + copy.updateMain(); + await ctx.penpot.waitForLayoutUpdate(); + await waitFor(() => childColor(main) === '#ff0000'); + expect(childColor(main)).toBe('#ff0000'); + expect(childColor(other)).toBe('#ff0000'); + + main.children[0].fills = [{ fillColor: '#00ff00', fillOpacity: 1 }]; + await waitFor(() => childColor(copy) === '#00ff00'); + expect(childColor(copy)).toBe('#00ff00'); + }); + + test('updateMain rejects plain shapes, main instances, and copy children', (ctx) => { + const rect = ctx.penpot.createRectangle(); + ctx.board.appendChild(rect); + expect(() => rect.updateMain()).toThrow(); + const comp = makeComponent(ctx); + expect(() => comp.mainInstance().updateMain()).toThrow(); + const copy = comp.instance() as Board; + ctx.board.appendChild(copy); + expect(() => copy.children[0].updateMain()).toThrow(); + }); + + test('deleted components reject duplication, annotation edits, and updateMain', async (ctx) => { + const comp = makeComponent(ctx); + const copy = comp.instance(); + ctx.board.appendChild(copy); + comp.remove(); + await ctx.penpot.waitForLayoutUpdate(); + expect(() => comp.duplicate()).toThrow('deleted component'); + expect(() => { + comp.annotation = 'Usage'; + }).toThrow('deleted component'); + expect(() => copy.updateMain()).toThrow('main component'); + }); + + test('updateMain rejects a copy on an inactive page and inside another copy', async (ctx) => { + const main = nestedComponentMain(ctx); + const outerCopy = main.component()?.instance() as Board; + ctx.board.appendChild(outerCopy); + expect(() => outerCopy.children[0].updateMain()).toThrow( + 'inside another component copy', + ); + + const original = ctx.penpot.currentPage; + expect(original).not.toBeNull(); + if (!original) return; + const page = ctx.penpot.createPage(); + try { + await ctx.penpot.openPage(page); + expect(() => outerCopy.updateMain()).toThrow('not currently active'); + } finally { + await ctx.penpot.openPage(original); + page.remove(); + } + }); + + test('component assets can be edited without activating their main instance page', async (ctx) => { + const original = ctx.penpot.currentPage; + expect(original).not.toBeNull(); + if (!original) return; + const comp = makeComponent(ctx); + const mainId = comp.mainInstance().id; + const page = ctx.penpot.createPage(); + try { + await ctx.penpot.openPage(page); + const selected = ctx.penpot.createRectangle(); + (page.root as Board).appendChild(selected); + ctx.penpot.selection = [selected]; + const zoom = ctx.penpot.viewport.zoom; + const center = ctx.penpot.viewport.center; + + comp.annotation = 'Edited from another page'; + const duplicate = comp.duplicate(); + await ctx.penpot.waitForLayoutUpdate(); + expect(original.getShapeById(duplicate.mainInstance().id)).not.toBeNull(); + expect(comp.annotation).toBe('Edited from another page'); + duplicate.remove(); + comp.remove(); + await ctx.penpot.waitForLayoutUpdate(); + expect(original.getShapeById(mainId)).toBeNull(); + expect(ctx.penpot.currentPage?.id).toBe(page.id); + expect(ctx.penpot.selection.map((shape) => shape.id)).toEqual([ + selected.id, + ]); + expect(ctx.penpot.viewport.zoom).toBeCloseTo(zoom); + expect(ctx.penpot.viewport.center.x).toBeCloseTo(center.x); + expect(ctx.penpot.viewport.center.y).toBeCloseTo(center.y); + } finally { + await ctx.penpot.openPage(original); + page.remove(); + } + }); +}); diff --git a/plugins/libs/plugin-types/index.d.ts b/plugins/libs/plugin-types/index.d.ts index 884f5002e5..66d73cc4af 100644 --- a/plugins/libs/plugin-types/index.d.ts +++ b/plugins/libs/plugin-types/index.d.ts @@ -2668,6 +2668,12 @@ export interface Library extends PluginData { */ readonly components: LibraryComponent[]; + /** + * Deleted components, including individual deleted variants. + * Use a component's `restore()` method to restore it. + */ + readonly deletedComponents: LibraryComponent[]; + /** * A catalog of Design Tokens in the library. * @@ -2763,6 +2769,39 @@ export interface LibraryColor extends LibraryElement { * This interface extends `LibraryElement` and includes properties specific to component elements. */ export interface LibraryComponent extends LibraryElement { + /** + * The component annotation, or null when none is set. + * Set null to delete it. Text can contain at most 300 characters. + * Writing requires `library:write` permission and a live component in the + * current file's library. + */ + annotation: string | null; + + /** + * Duplicates this component and its main instance in the current file's library. + * Requires `library:write` permission. The component must not be deleted. + */ + duplicate(): LibraryComponent; + + /** + * Deletes this component and its main instance from the current file's library. + * Requires `library:write` permission. Does nothing if already deleted. + * Surviving copies keep their link so the component can be restored later. + */ + remove(): void; + + /** + * Restores this component if it has been deleted, including its main instance. + * Leaves the current page, selection, zoom, and viewport unchanged. + * Requires `library:write` permission. Does nothing if already restored. + * + * @example + * ```js + * penpot.selection[0].component()?.restore(); + * ``` + */ + restore(): void; + /** * Creates an instance of the component. * @return Returns a `Shape` object representing the instance of the component. @@ -3980,6 +4019,15 @@ export interface ShapeBase extends PluginData { */ resetOverrides(): void; + /** + * Applies this copy's overrides to its main component and synchronizes copies. + * Similar to the editor's "update main component" action. + * Requires `content:write` and `library:write` permissions. + * The shape must be a component copy head on the active page of the current file, + * outside any other component copy, with a main component that is not deleted. + */ + updateMain(): void; + /** * Switch a VariantComponent copy to the nearest one that has the specified property value * @param pos The position of the property to update From ea45507ac82d9356f3e9a475fe9140ec525e378c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 14 Oct 2026 16:35:03 +0000 Subject: [PATCH 2/2] :wrench: Raise commit message limits to 80 chars Relax the subject limit from 70 to 80 characters and the body wrap limit from 76 to 80. Merge/Revert/Reapply commits skip the subject length check since git prepends those prefixes to an existing subject. Update the checker, its tests, and all related docs and memories accordingly. AI-assisted-by: muse-spark-1.3-contributor Signed-off-by: Andrey Antukh --- .agents/skills/create-commit/SKILL.md | 7 +-- .serena/memories/critical-info.md | 2 +- .serena/memories/workflow/creating-commits.md | 18 ++++---- .serena/memories/workflow/creating-prs.md | 2 +- AGENTS.md | 3 +- CONTRIBUTING.md | 8 ++-- scripts/check-commit | 29 ++++++++----- scripts/test_check_commit.py | 43 +++++++++++++------ 8 files changed, 71 insertions(+), 41 deletions(-) diff --git a/.agents/skills/create-commit/SKILL.md b/.agents/skills/create-commit/SKILL.md index bf0667ca65..dd85191fd9 100644 --- a/.agents/skills/create-commit/SKILL.md +++ b/.agents/skills/create-commit/SKILL.md @@ -23,10 +23,11 @@ subject/body limits, and the `AI-assisted-by` trailer. Follow it exactly. ## Iron Rules (non-negotiable) -1. **Wrap every body line at 76 characters or fewer.** Count characters, do +1. **Wrap every body line at 80 characters or fewer.** Count characters, do not eyeball. Exceptions: `Signed-off-by:` / `AI-assisted-by:` trailers and lines carrying a URL. This is the rule agents skip most often. -2. **Subject ≤70 chars**, imperative, capitalized, no trailing period. +2. **Subject ≤80 chars**, imperative, capitalized, no trailing period. + `Merge`/`Revert`/`Reapply` commits skip the length check. 3. **Blank line between subject and body.** 4. **Run `./scripts/check-commit` and require exit code 0.** It mechanically checks rules 1–3. A non-zero exit is a hard blocker: fix the message and @@ -41,7 +42,7 @@ subject/body limits, and the `AI-assisted-by` trailer. Follow it exactly. that does not match the stated intent, **STOP** and tell the user before committing. 3. Draft the message following the format in the memory doc, wrapping the body - at 76 characters per line, and run: + at 80 characters per line, and run: ```bash git commit -m "" -m "" ``` diff --git a/.serena/memories/critical-info.md b/.serena/memories/critical-info.md index 5e84c9834e..d0304e1fe8 100644 --- a/.serena/memories/critical-info.md +++ b/.serena/memories/critical-info.md @@ -12,7 +12,7 @@ You are working on the GitHub project `penpot/penpot`, a monorepo. # Development workflow - Commit/PR/issue creation is **on explicit request only**. Before any of these actions, read the relevant memory — don't infer format from prior examples: - - Before `git commit` → `mem:workflow/creating-commits` (subject/body format, 76-char body wrapping enforced by `scripts/check-commit`, `AI-assisted-by: model-name` trailer) + - Before `git commit` → `mem:workflow/creating-commits` (subject/body format, 80-char body wrapping enforced by `scripts/check-commit`, `AI-assisted-by: model-name` trailer) - Before `gh issue create` → `mem:workflow/creating-issues` (title derivation, body template, labels, Issue Type) - Before `gh pr create` / `gh pr edit` → `mem:workflow/creating-prs` (title format, body structure, "Note:" line) - Before a repo-wide pnpm version update → `mem:workflow/updating-pnpm` (workspace layout, `scripts/sync-pnpm-version` flow, the stamp-missing-field and ignored-builds gotchas, verification steps) diff --git a/.serena/memories/workflow/creating-commits.md b/.serena/memories/workflow/creating-commits.md index a60234819f..678681d4ba 100644 --- a/.serena/memories/workflow/creating-commits.md +++ b/.serena/memories/workflow/creating-commits.md @@ -11,12 +11,11 @@ automatically pull the identity from the local git config `user.name` and `user. ## Message Format ``` -:emoji: Subject line (imperative, capitalized, no period, <=70 chars) +:emoji: Subject line (imperative, capitalized, no period, <=80 chars) Body explaining what changed and why. -Wrap lines at 76 characters — git log adds a -four-space indent, so 76 + 4 fits an 80-column -terminal. Keep each line concise. +Wrap lines at 80 characters. +Keep each line concise. AI-assisted-by: model-name ``` @@ -26,16 +25,17 @@ AI-assisted-by: model-name These rules are not advisory. Do not commit until every one holds. A commit that breaks them is wrong, even if the code is right. -- **Body lines MUST wrap at 76 characters or fewer.** Measure every line; do - not eyeball it. This is the rule most often skipped. Rationale: `git log` - indents the body four spaces, so 76 + 4 fits an 80-column terminal. -- **Subject MUST be ≤70 chars**, imperative, capitalized, no trailing period. +- **Body lines MUST wrap at 80 characters or fewer.** Measure every line; do + not eyeball it. This is the rule most often skipped. +- **Subject MUST be ≤80 chars**, imperative, capitalized, no trailing period. + Merge/Revert/Reapply commits skip the length check: git prepends those + prefixes to an existing subject. - **MUST be a blank line** between subject and body. - **MUST run `scripts/check-commit` and get exit code 0 before finishing.** It mechanically validates the rules above; a failing run is a blocker. - It checks `HEAD` by default: `./scripts/check-commit` - For another commit: `./scripts/check-commit -c ` -- **NEVER** hand-wave the body as "one long line". If a line exceeds 76, +- **NEVER** hand-wave the body as "one long line". If a line exceeds 80, break it at a space. - **NEVER add a `Co-authored-by:` trailer.** It is forbidden; `scripts/check-commit` fails the commit when any line matches it (case-insensitive). - CI runs `scripts/check-commit` on every new commit (`mem:workflow/creating-commits` is enforced by `.github/workflows/commit-checker.yml`), so a forbidden trailer fails the build. diff --git a/.serena/memories/workflow/creating-prs.md b/.serena/memories/workflow/creating-prs.md index f9b41275ef..4d9d0fe8d4 100644 --- a/.serena/memories/workflow/creating-prs.md +++ b/.serena/memories/workflow/creating-prs.md @@ -27,7 +27,7 @@ Always add the PR to the Main project (`--project "Main"`) unless the user expli PR titles follow commit title conventions: ``` -:emoji: Subject line (imperative, capitalized, no period, <=70 chars) +:emoji: Subject line (imperative, capitalized, no period, <=80 chars) ``` See `mem:workflow/creating-commits` for emoji codes. Squash merge uses the PR title as the final commit subject, so title format matters. diff --git a/AGENTS.md b/AGENTS.md index fa95b4c045..e900e32518 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,8 @@ - **Read `AGENTS.local.md` after this file whenever it exists.** It carries the developer's steering for this project and overrides this file on prose style and workflow, but not on project policy, where instructions here prevail. -- **Commit message body lines MUST wrap at ≤76 chars** (subject ≤70 chars) and +- **Commit message body lines MUST wrap at ≤80 chars** (subject ≤80 chars, + except `Merge`/`Revert`/`Reapply` which skip the subject length check) and the commit MUST pass `./scripts/check-commit` with exit code 0 before you consider it done. This is mechanically checked — do not eyeball it. - **Read the workflow memory BEFORE the corresponding action**: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2d665b0715..9b23ae0fa9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -124,7 +124,7 @@ commit history — what matters is the [PR title](#title). Pull request titles **must** follow the same convention as commit subjects: ``` -:emoji: Subject line (imperative, capitalized, no period, <=70 chars) +:emoji: Subject line (imperative, capitalized, no period, <=80 chars) ``` Read [Creating Commits](./.serena/memories/workflow/creating-commits.md) @@ -220,8 +220,10 @@ Commit messages must follow this format: - Capitalize the first letter of the subject - Add clear and concise description on the body - Do not end the subject with a period -- Keep the subject to **70 characters** or fewer -- **Wrap body lines at 76 characters or fewer** (trailers and URLs excepted) +- Keep the subject to **80 characters** or fewer + (`Merge`/`Revert`/`Reapply` commits skip this limit, since git prepends + those prefixes to an existing subject) +- **Wrap body lines at 80 characters or fewer** (trailers and URLs excepted) - Separate the subject from the body with a **blank line** - Never add a `Co-authored-by` trailer — it is forbidden and fails `./scripts/check-commit` diff --git a/scripts/check-commit b/scripts/check-commit index d0e9f59899..df4672ea59 100755 --- a/scripts/check-commit +++ b/scripts/check-commit @@ -5,7 +5,7 @@ Check commit messages against Penpot's commit guidelines. Validates commit messages using the rules defined in: - .github/workflows/commit-checker.yml (regex pattern) - CONTRIBUTING.md (formatting rules, subject length, DCO) - - .serena/memories/workflow/creating-commits.md (body wrapped at 76 chars) + - .serena/memories/workflow/creating-commits.md (body wrapped at 80 chars) By default, checks HEAD. Use --commit to specify a different commit. @@ -39,12 +39,17 @@ COMMIT_PATTERN = re.compile( MERGE_PATTERN = re.compile(r"^(Merge|Revert|Reapply).+[^.]$") +# ── Subject line length ────────────────────────────────────────────────────── +# Subject lines must fit in 80 characters. Merge/Revert/Reapply commits are +# exempt: git prepends "Merge ..."/"Revert ..." to an existing subject, so +# enforcing the limit would fail on text the author did not write. +MAX_SUBJECT_LINE = 80 + # ── Body line wrapping ─────────────────────────────────────────────────────── -# Commit bodies must wrap at 76 characters (see -# .serena/memories/workflow/creating-commits.md). That leaves room for the -# four-space indent git log adds, fitting an 80-column terminal. Trailers and +# Commit bodies must wrap at 80 characters (see +# .serena/memories/workflow/creating-commits.md). Trailers and # URLs are exempt: they cannot be wrapped without losing meaning. -MAX_BODY_LINE = 76 +MAX_BODY_LINE = 80 TRAILER_PATTERN = re.compile( r"^(Signed-off-by|Co-developed-by|Reviewed-by|" @@ -110,11 +115,13 @@ def check_regex(message): def check_subject_length(message): - """Subject line must be ≤ 70 characters.""" + """Subject line must be ≤ 80 characters (Merge/Revert/Reapply exempt).""" first_line = message.split("\n")[0] - if len(first_line) > 70: + if MERGE_PATTERN.match(first_line): + return True, None + if len(first_line) > MAX_SUBJECT_LINE: return False, ( - f"Subject line exceeds 70 characters ({len(first_line)} chars):\n" + f"Subject line exceeds {MAX_SUBJECT_LINE} characters ({len(first_line)} chars):\n" f" {first_line}" ) return True, None @@ -166,7 +173,7 @@ def check_body_blank_line(message): def check_body_line_length(message): - """Body lines must wrap at 76 characters or fewer. + """Body lines must wrap at 80 characters or fewer. The subject (first line) has its own length rule. Trailers (e.g. Signed-off-by) and lines carrying a URL are exempt, since wrapping them @@ -250,11 +257,11 @@ def main(): validators = [ ("Regex pattern", check_regex), - ("Subject ≤ 70 chars", check_subject_length), + ("Subject ≤ 80 chars", check_subject_length), ("No trailing period in subject", check_subject_no_trailing_dot), ("Subject capitalized", check_subject_capitalized), ("Blank line after subject", check_body_blank_line), - ("Body wrapped at 76 chars", check_body_line_length), + ("Body wrapped at 80 chars", check_body_line_length), ("No Co-authored-by trailer", check_no_co_authored_by), ] diff --git a/scripts/test_check_commit.py b/scripts/test_check_commit.py index a3a7a1c2b9..037bc8fd56 100644 --- a/scripts/test_check_commit.py +++ b/scripts/test_check_commit.py @@ -55,20 +55,20 @@ class BodyLineLengthTests(unittest.TestCase): self.assert_ok(message) def test_line_at_limit_passes(self): - line = "x " * 38 # 76 chars, breakable - self.assertEqual(len(line), 76) + line = "x " * 40 # 80 chars, breakable + self.assertEqual(len(line), 80) self.assert_ok(":bug: Fix crash\n\n" + line + "\n") def test_line_one_over_limit_fails(self): - line = "x " * 38 + "x" # 77 chars, breakable - self.assertEqual(len(line), 77) + line = "x " * 40 + "x" # 81 chars, breakable + self.assertEqual(len(line), 81) error = self.assert_fail(":bug: Fix crash\n\n" + line + "\n") - self.assertIn("76", error) + self.assertIn("80", error) def test_long_body_line_fails(self): long_line = "word " * 20 # 100 chars, breakable error = self.assert_fail(":bug: Fix crash\n\n" + long_line + "\n") - self.assertIn("76", error) + self.assertIn("80", error) self.assertIn("line 3", error) def test_subject_is_not_checked(self): @@ -150,15 +150,34 @@ class SubjectRulesRegressionTests(unittest.TestCase): ok, _ = check_commit.check_subject_no_trailing_dot(":bug: Fix crash.") self.assertFalse(ok) - def test_subject_at_70_chars_passes(self): - # ":bug: " is 6 chars, so 64 chars of text reach exactly 70. - ok, error = check_commit.check_subject_length(":bug: " + "S" * 64) + def test_subject_at_80_chars_passes(self): + # ":bug: " is 6 chars, so 74 chars of text reach exactly 80. + ok, error = check_commit.check_subject_length(":bug: " + "S" * 74) self.assertTrue(ok, error) - def test_subject_over_70_chars_fails(self): - ok, error = check_commit.check_subject_length(":bug: " + "S" * 65) + def test_subject_over_80_chars_fails(self): + ok, error = check_commit.check_subject_length(":bug: " + "S" * 75) self.assertFalse(ok) - self.assertIn("70", error) + self.assertIn("80", error) + + def test_merge_subject_skips_length_check(self): + # Git generates Merge subjects from branch names; the limit is skipped. + subject = "Merge branch 'some-very-long-branch-name-that-keeps-going' into develop with extra text here" + self.assertGreater(len(subject), 80) + ok, error = check_commit.check_subject_length(subject) + self.assertTrue(ok, error) + + def test_revert_subject_skips_length_check(self): + subject = 'Revert ":bug: Fix some earlier commit with a fairly long subject line already here" extra' + self.assertGreater(len(subject), 80) + ok, error = check_commit.check_subject_length(subject) + self.assertTrue(ok, error) + + def test_reapply_subject_skips_length_check(self): + subject = 'Reapply ":bug: Fix some earlier commit with a fairly long subject line already here" extra' + self.assertGreater(len(subject), 80) + ok, error = check_commit.check_subject_length(subject) + self.assertTrue(ok, error) if __name__ == "__main__":