From b598d7d72ecc6b926469cef6d2a9d642759d966e Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Fri, 11 Sep 2026 13:19:55 +0200 Subject: [PATCH] :bug: Fix problem in plugins api when removing interactions (#11621) --- .../app/common/types/shape/interactions.cljc | 13 +- .../types/shape_interactions_test.cljc | 15 +- frontend/src/app/plugins/shape.cljs | 165 ++++++++++-------- frontend/src/app/plugins/utils.cljs | 9 + plugins/CHANGELOG.md | 2 + .../src/tests/interactions.test.ts | 64 +++++++ 6 files changed, 190 insertions(+), 78 deletions(-) diff --git a/common/src/app/common/types/shape/interactions.cljc b/common/src/app/common/types/shape/interactions.cljc index 6b06b68897..abbbf3fffe 100644 --- a/common/src/app/common/types/shape/interactions.cljc +++ b/common/src/app/common/types/shape/interactions.cljc @@ -712,14 +712,21 @@ (conj (or interactions []) interaction)) (defn remove-interaction + "Interactions without the one at `index`; unchanged when `index` addresses none." [interactions index] (let [interactions (or interactions [])] - (into (subvec interactions 0 index) - (subvec interactions (inc index))))) + (if (and (int? index) (< -1 index (count interactions))) + (into (subvec interactions 0 index) + (subvec interactions (inc index))) + interactions))) (defn update-interaction + "Interactions with `update-fn` applied at `index`; unchanged when `index` + addresses none." [interactions index update-fn] - (update interactions index update-fn)) + (if (and (int? index) (< -1 index (count interactions))) + (update interactions index update-fn) + interactions)) (defn remap-interactions "Update all interactions whose destination points to a shape in the diff --git a/common/test/common_tests/types/shape_interactions_test.cljc b/common/test/common_tests/types/shape_interactions_test.cljc index da056ae136..00ea23dbcd 100644 --- a/common/test/common_tests/types/shape_interactions_test.cljc +++ b/common/test/common_tests/types/shape_interactions_test.cljc @@ -858,7 +858,20 @@ (t/testing "Update interaction" (let [new-interactions (ctsi/update-interaction interactions 1 #(ctsi/set-action-type % :open-url))] (t/is (= (count new-interactions) 2)) - (t/is (= (:action-type (last new-interactions)) :open-url)))))) + (t/is (= (:action-type (last new-interactions)) :open-url)))) + + (t/testing "Remove interaction with an index out of range" + (t/is (= interactions (ctsi/remove-interaction interactions 2))) + (t/is (= interactions (ctsi/remove-interaction interactions -1))) + (t/is (= interactions (ctsi/remove-interaction interactions nil))) + (t/is (= [] (ctsi/remove-interaction nil 0)))) + + (t/testing "Update interaction with an index out of range" + (let [update-fn #(ctsi/set-action-type % :open-url)] + (t/is (= interactions (ctsi/update-interaction interactions 2 update-fn))) + (t/is (= interactions (ctsi/update-interaction interactions -1 update-fn))) + (t/is (= interactions (ctsi/update-interaction interactions nil update-fn))) + (t/is (nil? (ctsi/update-interaction nil 0 update-fn))))))) (t/deftest remap-interactions diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 9031f021c0..81bcf89628 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -80,89 +80,102 @@ (obj/type-of? p "InteractionProxy")) (defn interaction-proxy - [plugin-id file-id page-id shape-id index] - (obj/reify {:name "InteractionProxy"} - :$plugin {:enumerable false :get (fn [] plugin-id)} - :$file {:enumerable false :get (fn [] file-id)} - :$page {:enumerable false :get (fn [] page-id)} - :$shape {:enumerable false :get (fn [] shape-id)} - :$index {:enumerable false :get (fn [] index)} + "Proxy over one interaction of a shape. - ;; Not enumerable so we don't have an infinite loop - :shape - {:enumerable false - :get (fn [] (shape-proxy plugin-id file-id page-id shape-id))} + Interactions are addressed by position, which shifts as interactions are added + or removed, so the position is resolved on each access from `interaction`, + kept up to date with the writes made through the proxy." + [plugin-id file-id page-id shape-id interaction index] + (let [current (atom interaction) + locate-index (fn [] (u/locate-interaction-index file-id page-id shape-id @current index))] + (obj/reify {:name "InteractionProxy"} + :$plugin {:enumerable false :get (fn [] plugin-id)} + :$file {:enumerable false :get (fn [] file-id)} + :$page {:enumerable false :get (fn [] page-id)} + :$shape {:enumerable false :get (fn [] shape-id)} + :$index {:enumerable false :get locate-index} - :trigger - {:this true - :get #(-> % u/proxy->interaction :event-type format/format-key) - :set - (fn [_ value] - (let [value (parser/parse-keyword value)] + ;; Not enumerable so we don't have an infinite loop + :shape + {:enumerable false + :get (fn [] (shape-proxy plugin-id file-id page-id shape-id))} + + :trigger + {:this true + :get #(-> % u/proxy->interaction :event-type format/format-key) + :set + (fn [_ value] + (let [value (parser/parse-keyword value)] + (cond + (not (contains? ctsi/event-types value)) + (u/not-valid plugin-id :trigger value) + + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :trigger "Plugin doesn't have 'content:write' permission") + + :else + (do + (st/emit! (dwi/update-interaction + (u/locate-shape file-id page-id shape-id) + (locate-index) + #(assoc % :event-type value) + {:page-id page-id})) + (swap! current assoc :event-type value)))))} + + :delay + {:this true + :get #(-> % u/proxy->interaction :delay) + :set + (fn [_ value] (cond - (not (contains? ctsi/event-types value)) - (u/not-valid plugin-id :trigger value) + (or (not (sm/valid-safe-int? value)) (neg? value)) + (u/not-valid plugin-id :delay value) (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :trigger "Plugin doesn't have 'content:write' permission") + (u/not-valid plugin-id :delay "Plugin doesn't have 'content:write' permission") :else - (st/emit! (dwi/update-interaction - (u/locate-shape file-id page-id shape-id) - index - #(assoc % :event-type value) - {:page-id page-id})))))} + (do + (st/emit! (dwi/update-interaction + (u/locate-shape file-id page-id shape-id) + (locate-index) + #(assoc % :delay value) + {:page-id page-id})) + (swap! current assoc :delay value))))} - :delay - {:this true - :get #(-> % u/proxy->interaction :delay) - :set - (fn [_ value] - (cond - (or (not (sm/valid-safe-int? value)) (neg? value)) - (u/not-valid plugin-id :delay value) + :action + {:this true + :get #(-> % u/proxy->interaction (format/format-action plugin-id file-id page-id)) + :set + (fn [self value] + (let [params (parser/parse-action value) + interaction + (-> (u/proxy->interaction self) + (d/patch-object params))] + (cond + (not (sm/validate ctsi/schema:interaction interaction)) + (u/not-valid plugin-id :action interaction) - (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :delay "Plugin doesn't have 'content:write' permission") + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :action "Plugin doesn't have 'content:write' permission") - :else - (st/emit! (dwi/update-interaction - (u/locate-shape file-id page-id shape-id) - index - #(assoc % :delay value) - {:page-id page-id}))))} + :else + (do + (st/emit! (dwi/update-interaction + (u/locate-shape file-id page-id shape-id) + (locate-index) + #(d/patch-object % params) + {:page-id page-id})) + (reset! current interaction)))))} - :action - {:this true - :get #(-> % u/proxy->interaction (format/format-action plugin-id file-id page-id)) - :set - (fn [self value] - (let [params (parser/parse-action value) - interaction - (-> (u/proxy->interaction self) - (d/patch-object params))] - (cond - (not (sm/validate ctsi/schema:interaction interaction)) - (u/not-valid plugin-id :action interaction) + :remove + (fn [] + (cond + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :remove "Plugin doesn't have 'content:write' permission") - (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :action "Plugin doesn't have 'content:write' permission") - - :else - (st/emit! (dwi/update-interaction - (u/locate-shape file-id page-id shape-id) - index - #(d/patch-object % params) - {:page-id page-id})))))} - - :remove - (fn [] - (cond - (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :remove "Plugin doesn't have 'content:write' permission") - - :else - (st/emit! (dwi/remove-interaction {:id shape-id} index)))))) + :else + (st/emit! (dwi/remove-interaction {:id shape-id} (locate-index)))))))) (def lib-typography-proxy? nil) (def lib-component-proxy nil) @@ -980,8 +993,9 @@ (fn [self] (let [interactions (-> self u/proxy->shape :interactions)] (format/format-array - #(interaction-proxy plugin-id file-id page-id id %) - (range 0 (count interactions)))))} + (fn [[index interaction]] + (interaction-proxy plugin-id file-id page-id id interaction index)) + (d/enumerate interactions))))} ;; Methods :resize @@ -1626,7 +1640,7 @@ (st/emit! (dwi/add-interaction page-id id interaction) (se/event plugin-id "add-interaction")) - (interaction-proxy plugin-id file-id page-id id index))))) + (interaction-proxy plugin-id file-id page-id id interaction index))))) :removeInteraction (fn [interaction] @@ -1637,6 +1651,9 @@ (not (r/check-permission plugin-id "content:write")) (u/not-valid plugin-id :removeInteraction "Plugin doesn't have 'content:write' permission") + (not= id (obj/get interaction "$shape")) + (u/not-valid plugin-id :removeInteraction "The interaction doesn't belong to this shape") + :else (st/emit! (dwi/remove-interaction {:id id} (obj/get interaction "$index")) diff --git a/frontend/src/app/plugins/utils.cljs b/frontend/src/app/plugins/utils.cljs index d6d921ea8a..9c8dbc4a5a 100644 --- a/frontend/src/app/plugins/utils.cljs +++ b/frontend/src/app/plugins/utils.cljs @@ -206,6 +206,15 @@ (when-let [shape (locate-shape file-id page-id shape-id)] (get-in shape [:interactions index]))) +(defn locate-interaction-index + "Position of `interaction` within the shape's current interactions, falling + back to `index` while it addresses an existing interaction." + [file-id page-id shape-id interaction index] + (let [interactions (-> (locate-shape file-id page-id shape-id) :interactions)] + (or (d/index-of interactions interaction) + (when (and (int? index) (< -1 index (count interactions))) + index)))) + (defn proxy->interaction [proxy] (let [file-id (obj/get proxy "$file") diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 9cd186baf0..7e0cd7491b 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -8,6 +8,8 @@ ### 🩹 Fixes +- **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**: `Library.createComponent()` now rejects invalid input (an empty shape list, or a shape inside a component copy) with a validation error instead of returning a component proxy pointing at nothing. - **plugins-runtime**: Setting an individual padding/margin side (`leftPadding`, `topMargin`, …) now re-derives the padding/margin type, switching to `multiple` when the four sides stop being symmetric (so the value is actually painted) and back to `simple` once top/bottom and left/right are mirrored again. diff --git a/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts index fe18ca4e41..6f01f8ef0f 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts +++ b/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts @@ -349,6 +349,70 @@ describe('Interactions', () => { expect(r.interactions.length).toBe(before - 1); }); + // Removing an interaction shifts the ones after it, so draining a shape from + // a single read of the array must reach every interaction it returned. Both + // removal entry points are covered. + test('every interaction can be removed from one read of the array', async (ctx) => { + const r = rect(ctx); + r.addInteraction('click', { type: 'open-url', url: 'https://a.example' }); + await ctx.penpot.waitForLayoutUpdate(); + r.addInteraction('mouse-enter', { + type: 'open-url', + url: 'https://b.example', + }); + await ctx.penpot.waitForLayoutUpdate(); + expect(r.interactions).toHaveLength(2); + + for (const interaction of r.interactions) { + interaction.remove(); + await ctx.penpot.waitForLayoutUpdate(); + } + expect(r.interactions).toHaveLength(0); + }); + + test('removeInteraction can drain a shape from one read of the array', async (ctx) => { + const r = rect(ctx); + r.addInteraction('click', { type: 'open-url', url: 'https://a.example' }); + await ctx.penpot.waitForLayoutUpdate(); + r.addInteraction('mouse-enter', { + type: 'open-url', + url: 'https://b.example', + }); + await ctx.penpot.waitForLayoutUpdate(); + expect(r.interactions).toHaveLength(2); + + for (const interaction of r.interactions) { + r.removeInteraction(interaction); + await ctx.penpot.waitForLayoutUpdate(); + } + expect(r.interactions).toHaveLength(0); + }); + + // A held interaction addresses itself rather than a position, so a write + // reaches it even once an earlier interaction has shifted it. + test('an interaction still writes to itself after an earlier one is removed', async (ctx) => { + const r = rect(ctx); + for (const trigger of ['click', 'mouse-enter', 'mouse-leave'] as const) { + r.addInteraction(trigger, { + type: 'open-url', + url: `https://${trigger}.example`, + }); + await ctx.penpot.waitForLayoutUpdate(); + } + const [first, , last] = r.interactions; + + first.remove(); + await ctx.penpot.waitForLayoutUpdate(); + last.delay = 500; + await ctx.penpot.waitForLayoutUpdate(); + + expect(r.interactions.map((i) => i.trigger)).toEqual([ + 'mouse-enter', + 'mouse-leave', + ]); + expect(r.interactions.map((i) => i.delay)).toEqual([null, 500]); + }); + test('interaction trigger can be changed', (ctx) => { const dest = board(ctx); const r = rect(ctx);