🐛 Fix problem in plugins api when removing interactions

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

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

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"))

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")

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.
 

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);