github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Check and commit plugin component calls as the UI does

Browse Source 
- The UI offers swap and reset only on copy heads, and detach only on
  copies that are not inside another copy. The plugin's swapComponent
  and resetOverrides accepted any shape inside a copy (F92), and its
  detach any shape at all (F93), and sent the same events, whose
  changes expect a head. swapComponent and resetOverrides report "The
  shape is not a component copy head" for anything else; detach
  reports a shape that is not a copy, or a copy inside another copy,
  by ctn/detachable?, which the detach event checks too. swapComponent
  looked up the target file before checking its argument, so a value
  that is not a component failed an assert instead of reporting
  "Component not valid".
- The plugin's switchVariant did not check whether the switch would
  nest a component in itself. The switch event checks that and calls
  the :on-error of its params' metadata; the plugin passed none, so
  the default, rx/throw, built an error observable that nothing used,
  and the call did nothing and reported nothing (F53). The plugin
  passes an :on-error that reports "The switch would create a
  component nesting loop", and the event's default does nothing.
- Choosing a property value in the design tab with several members
  selected sent, for each member, an event that set the value and
  another that cleared its error, each its own commit and undo entry.
  The plugin's setVariantProperty set the value and left the error as
  it was (F94). dwv/update-property-value takes the components, and
  sets the value and clears the error of each in one commit; the
  design tab passes the selected members, the plugin its component,
  and the composable interpreter the one member of its edit.

shape-bugfixes-test checks each plugin case on the proxy, and
workspace-variants-test sets a value on two members with errors.

AI-assisted-by: claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
alonso.torres 2026-10-14 23:44:28 +02:00
parent fe91c9a4a0
commit a5ad3caffe
9 changed files with 153 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
common/src/app/common/types/container.cljc
View File

@ -414,6 +414,13 @@
true
(has-any-copy-parent? objects (:parent-id shape))))))
(defn detachable?
"Whether the shape can be detached from its component: a copy shape that
is not inside another copy."
[objects shape]
(and (ctk/in-component-copy? shape)
(not (has-any-copy-parent? objects shape))))
(defn has-any-main?
"Check if the shape is a main component or has any children or parent that is a main component."
[objects shape]

10
frontend/src/app/main/data/workspace/libraries.cljs
View File

@ -616,14 +616,14 @@
(watch [it state _]
(let [page-id (:current-page-id state)
file-id (:current-file-id state)
objects (dsh/lookup-page-objects state page-id)
fdata (dsh/lookup-file-data state file-id)
libraries (dsh/lookup-libraries state)
libraries (dsh/lookup-libraries state)]
changes (-> (pcb/empty-changes it)
(cli/generate-detach-component id fdata page-id libraries))]
(rx/of (dch/commit-changes changes))))))
(when (ctn/detachable? objects (get objects id))
(rx/of (dch/commit-changes (-> (pcb/empty-changes it)
(cli/generate-detach-component id fdata page-id libraries)))))))))
(defn detach-components
"Remove all references to components in the shapes with the given ids"

21
frontend/src/app/main/data/workspace/variants.cljs
View File

@ -109,13 +109,19 @@
(ev/event {::ev/name "variant-edit-property-name" ::ev/origin trigger})))))))
(defn update-property-value
"Updates the variant property value on the position pos in a component"
[component-id pos value]
"Set the value of the variant property at `pos` of the components
`component-ids`, and clear their errors."
[component-ids pos value]
(ptk/reify ::update-property-value
ptk/WatchEvent
(watch [it state _]
(rx/of (dch/commit-changes (-> (variant-changes it state)
(clvp/generate-update-property-value component-id pos value)))))))
(rx/of (dch/commit-changes
(reduce (fn [changes id]
(-> changes
(clvp/generate-update-property-value id pos value)
(clvp/generate-set-variant-error id nil)))
(variant-changes it state)
component-ids))))))
(defn update-error
"Sets or unsets an error for a component"
@ -442,7 +448,10 @@
(rx/of (combine-as-variants selected options))))))
(defn- variant-switch
"Switch the shape (that must be a variant copy head) for the closest one with the property value passed as parameter"
"Switch the shape (that must be a variant copy head) for the closest one
with the property value passed as parameter. A switch that would nest a
component in itself does nothing and calls the `:on-error` of the
metadata of `params`."
[shape {:keys [pos val] :as params}]
(ptk/reify ::variant-switch
ptk/WatchEvent
@ -451,7 +460,7 @@
nearest-comp (clv/find-switch-target libraries shape pos val)
{:keys [on-error]
:or {on-error rx/throw}} (meta params)]
:or {on-error (constantly nil)}} (meta params)]
;; Nothing to do when the value is already val or no variant has it
(when nearest-comp
(if (clv/swap-nesting-loop? (dsh/lookup-page-objects state)

10
frontend/src/app/main/ui/workspace/sidebar/options/menus/component.cljs
View File

@ -391,11 +391,11 @@
(mf/deps component-ids)
(fn [pos value]
(let [value (ctv/normalize-property-text (d/nilv value ""))]
(doseq [id component-ids]
(st/emit!
(ev/event {::ev/name "variant-edit-property-value" ::ev/origin "workspace:combo-design-tab"})
(dwv/update-property-value id pos value))
(st/emit! (dwv/update-error id))))))
(apply st/emit!
(dwv/update-property-value component-ids pos value)
(repeat (count component-ids)
(ev/event {::ev/name "variant-edit-property-value"
::ev/origin "workspace:combo-design-tab"}))))))
update-property-name
(mf/use-fn

2
frontend/src/app/plugins/library.cljs
View File

@ -1029,7 +1029,7 @@
:else
(st/emit!
(se/event plugin-id "variant-edit-property-value")
(dwv/update-property-value id pos (ctv/normalize-property-text value))))))))
(dwv/update-property-value [id] pos (ctv/normalize-property-text value))))))))
(defn library-proxy? [p]
(obj/type-of? p "LibraryProxy"))

24
frontend/src/app/plugins/shape.cljs
View File

@ -333,6 +333,10 @@
:else
(st/emit! (dwsh/update-shapes [id] #(assoc % :exports value))))))
(defn- copy-head?
[shape]
(and (ctk/instance-head? shape) (ctk/in-component-copy? shape)))
(defn shape-proxy? [p]
(obj/type-of? p "ShapeProxy"))
@ -1556,6 +1560,9 @@
(not (u/page-active? page-id))
(u/not-valid plugin-id :detach "Cannot modify a page that is not currently active")
(not (ctn/detachable? (u/locate-objects file-id page-id) (u/locate-shape file-id page-id id)))
(u/not-valid plugin-id :detach "Only a copy that is not inside another copy can be detached")
:else
(st/emit! (dwl/detach-component id))))
@ -1566,7 +1573,7 @@
valid-component? (obj/type-of? component "LibraryComponentProxy")
target-file (when valid-component? (obj/get component "$file"))
target-id (when valid-component? (obj/get component "$id"))
target-data (some-> (u/locate-file target-file) :data)]
target-data (some-> target-file u/locate-file :data)]
(cond
(not (r/check-permission plugin-id "content:write"))
(u/not-valid plugin-id :swapComponent "Plugin doesn't have 'content:write' permission")
@ -1574,12 +1581,12 @@
(not (u/page-active? page-id))
(u/not-valid plugin-id :swapComponent "Cannot modify a page that is not currently active")
(not (copy-head? shape))
(u/not-valid plugin-id :swapComponent "The shape is not a component copy head")
(not valid-component?)
(u/not-valid plugin-id :swapComponent "Component not valid")
(not (ctk/in-component-copy? shape))
(u/not-valid plugin-id :swapComponent "The shape is not a component copy instance")
(clv/swap-nesting-loop? objects shape target-data target-id)
(u/not-valid plugin-id :swapComponent "The swap would create a component nesting loop")
@ -1599,8 +1606,8 @@
(not (u/page-active? page-id))
(u/not-valid plugin-id :resetOverrides "Cannot modify a page that is not currently active")
(not (ctk/in-component-copy? shape))
(u/not-valid plugin-id :resetOverrides "The shape is not a component copy instance")
(not (copy-head? shape))
(u/not-valid plugin-id :resetOverrides "The shape is not a component copy head")
:else
(st/emit! (dwl/reset-component id)))))
@ -1847,7 +1854,10 @@
:else
(let [shape (u/locate-shape file-id page-id id)]
(if (clv/valid-switch? (dsh/lookup-libraries @st/state) shape pos value)
(st/emit! (-> (dwv/variants-switch {:shapes [shape] :pos pos :val value})
(st/emit! (-> (dwv/variants-switch
(with-meta {:shapes [shape] :pos pos :val value}
{:on-error #(u/not-valid plugin-id :switchVariant
"The switch would create a component nesting loop")}))
(se/add-event plugin-id)))
(u/not-valid plugin-id :switchVariant "Shape, property, or value is not valid")))))

3
frontend/test/frontend_tests/composable_tests/interpreter.cljs
View File

@ -237,7 +237,8 @@
:rename (let [[variant-id pos name] args]
(dwv/update-property-name variant-id pos name {:trigger "workspace:design-tab"}))
:reorder (apply dwv/reorder-variant-properties args)
:set-value (apply dwv/update-property-value args)
:set-value (let [[component-id pos value] args]
(dwv/update-property-value [component-id] pos value))
;; the layers panel's rename of the member's main
:rename-in-layers (let [{:keys [set-name params]} op]
(dw/rename-shape-or-variant (:id (n/member-root situation set-name (:member params)))

16
frontend/test/frontend_tests/data/workspace_variants_test.cljs
View File

@ -227,3 +227,19 @@
(let [data (:data (ths/get-file-from-state state))]
(t/is (= (remove #{"Property 6"} (map :name (props "a")))
(map :name (:variant-properties (ctkl/get-component data (cthi/id :c01))))))))))))
(t/deftest set-a-property-value-on-several-members
(t/async
done
(let [file (-> (setup-file)
(cths/update-shape :m01 :variant-error "Duplicated")
(cths/update-shape :m02 :variant-error "Duplicated"))]
(ths/run-store
(ths/setup-store file) done
[(dwv/update-property-value [(cthi/id :c01) (cthi/id :c02)] 0 "Same")]
(fn [state]
(let [file' (ths/get-file-from-state state)
value #(get-in (ctkl/get-component (:data file') (cthi/id %)) [:variant-properties 0 :value])]
(t/is (= ["Same" "Same"] [(value :c01) (value :c02)]))
(t/is (nil? (:variant-error (cths/get-shape file' :m01))))
(t/is (nil? (:variant-error (cths/get-shape file' :m02))))))))))

85
frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs
View File

@ -7,7 +7,10 @@
(ns frontend-tests.plugins.shape-bugfixes-test
(:require
[app.common.data :as d]
[app.common.test-helpers.components :as cthc]
[app.common.test-helpers.files :as cthf]
[app.common.test-helpers.ids-map :as cthi]
[app.common.test-helpers.variants :as cthv]
[app.common.types.component :as ctk]
[app.common.uuid :as uuid]
[app.main.data.workspace :as dw]
@ -410,3 +413,85 @@
(t/is (= 1 (count @errors)))
(t/is (= [plugin-id :combineAsVariants "Plugin doesn't have 'content:write' permission"]
(first @errors)))))))
(defn- plugin-errors-of
"The errors the plugin reports when `call` runs on the proxy of the shape
`shape-id` of `objects`, with every check passing but the shape's own."
[objects shape-id call]
(let [file-id (uuid/next)
page-id (uuid/next)
errors (atom [])
emitted (atom [])]
(with-redefs [u/locate-shape (fn [_ _ id] (get objects id))
u/locate-objects (fn ([] objects) ([_ _] objects))
u/page-active? (constantly true)
r/check-permission (constantly true)
u/not-valid (mock/stub (fn [_ prop msg] (swap! errors conj [prop msg])))
st/emit! (mock/stub (fn [& events] (swap! emitted into events)))]
(call (shape/shape-proxy plugin-id file-id page-id shape-id)))
{:errors @errors :emitted @emitted}))
(defn- nested-copy-objects
"A copy root holding a nested copy head, which holds a child."
[]
(let [root (uuid/next)
head (uuid/next)
child (uuid/next)]
{:root root
:head head
:child child
:objects {root {:id root :type :frame :parent-id uuid/zero :shapes [head]
:component-id (uuid/next) :component-file (uuid/next)
:component-root true :shape-ref (uuid/next)}
head {:id head :type :frame :parent-id root :shapes [child]
:component-id (uuid/next) :component-file (uuid/next)
:shape-ref (uuid/next)}
child {:id child :type :rect :parent-id head :shape-ref (uuid/next)}}}))
(t/deftest swap-component-rejects-a-shape-that-is-no-copy-head
(let [{:keys [objects child]} (nested-copy-objects)
{:keys [errors emitted]} (plugin-errors-of objects child #(.swapComponent ^js % #js {}))]
(t/is (= [[:swapComponent "The shape is not a component copy head"]] errors))
(t/is (empty? emitted))))
(t/deftest reset-overrides-rejects-a-shape-that-is-no-copy-head
(let [{:keys [objects child]} (nested-copy-objects)
{:keys [errors emitted]} (plugin-errors-of objects child #(.resetOverrides ^js %))]
(t/is (= [[:resetOverrides "The shape is not a component copy head"]] errors))
(t/is (empty? emitted))))
(t/deftest detach-rejects-a-copy-nested-in-a-copy
(let [{:keys [objects head]} (nested-copy-objects)
{:keys [errors emitted]} (plugin-errors-of objects head #(.detach ^js %))]
(t/is (= [[:detach "Only a copy that is not inside another copy can be detached"]] errors))
(t/is (empty? emitted))))
(t/deftest swap-component-rejects-a-value-that-is-no-component
(let [{:keys [objects head]} (nested-copy-objects)
{:keys [errors emitted]} (plugin-errors-of objects head #(.swapComponent ^js % #js {}))]
(t/is (= [[:swapComponent "Component not valid"]] errors))
(t/is (empty? emitted))))
(t/deftest switch-variant-into-a-nesting-loop-reports-it
(t/async
done
(let [file (-> (cthf/sample-file :file1)
(cthv/add-variant :v01 :c01 :m01 :c02 :m02)
(cthc/instantiate-component :c01 :nested :parent-label :m02))
store (ths/setup-store file)
errors (atom [])]
(set! st/state store)
(mock/with-mocks
{u/not-valid (fn [_ prop msg] (swap! errors conj [prop msg]))
r/check-permission (constantly true)
u/page-active? (constantly true)}
(fn [done']
(.switchVariant (shape/shape-proxy plugin-id (:id file) (cthf/current-page-id file)
(cthi/id :nested))
0 "Value2")
(js/setTimeout
(fn []
(t/is (= [[:switchVariant "The switch would create a component nesting loop"]] @errors))
(done'))
50))
done))))