🐛 Fix problem with plugins api event handler (#11787)

frontend/src/app/plugins/events.cljs

@@ -17,7 +17,7 @@
    [app.util.theme :as theme]
    [goog.functions :as gf]))
 
-(defmulti handle-state-change (fn [type _] type))
+(defmulti handle-state-change (fn [type _ _ _ _] type))
 
 (defmethod handle-state-change "finish"
   [_ _ old-val new-val _]
@@ -65,14 +65,14 @@
       new-theme)))
 
 (defmethod handle-state-change "shapechange"
-  [_ plugin-id old-val new-val props]
-  (if-let [shape-id (-> (obj/get props "shapeId") parser/parse-id)]
+  [_ plugin-id old-val new-val {:keys [shape-id]}]
+  (if (some? shape-id)
     (let [old-shape (dsh/lookup-shape old-val shape-id)
           new-shape (dsh/lookup-shape new-val shape-id)
 
           file-id (:current-file-id new-val)
           page-id (:current-page-id new-val)]
-      (if (and (identical? old-shape new-shape) (some? plugin-id) (some? file-id) (some? page-id) (some? shape-id))
+      (if (and (identical? old-shape new-shape) (some? plugin-id) (some? file-id) (some? page-id))
         ::not-changed
         (shape/shape-proxy plugin-id file-id page-id shape-id)))
     ::not-changed))
@@ -86,12 +86,19 @@
       ::not-changed)))
 
 (defmethod handle-state-change :default
-  [_ _ _ _]
+  [_ _ _ _ _]
   ::not-changed)
 
+(defn- parse-props
+  "Resolves the listener options. Runs at registration, so a malformed
+  value raises where the plugin can see it."
+  [props]
+  {:shape-id (-> (obj/get props "shapeId") parser/parse-id)})
+
 (defn add-listener
   [type plugin-id callback props]
   (let [plugin-id (parser/parse-id plugin-id)
+        props (parse-props props)
         key (js/Symbol)
 
         ;; We wrap the callback in an exception handler so the plugins
@@ -111,9 +118,14 @@
     (add-watch
      st/state key
      (fn [_ _ old-val new-val]
-       (let [result (handle-state-change type plugin-id old-val new-val props)]
-         (when (not= ::not-changed result)
-           (debounced-callback result)))))
+       ;; The store notifies its watches with a plain iteration: a throw
+       ;; here starves the watches registered after this one.
+       (try
+         (let [result (handle-state-change type plugin-id old-val new-val props)]
+           (when (not= ::not-changed result)
+             (debounced-callback result)))
+         (catch :default cause
+           (.error js/console cause)))))
 
     ;; return the generated key
     key))

frontend/src/app/plugins/parser.cljs

@@ -15,8 +15,11 @@
    [cuerdas.core :as str]))
 
 (defn parse-id
+  "Parses an id from the plugin API. A blank value yields nil, since JS
+  callers pass an empty string where there is no id."
   [id]
-  (when id (uuid/parse id)))
+  (when-not (str/blank? id)
+    (uuid/parse id)))
 
 (defn parse-keyword
   [kw]

frontend/test/frontend_tests/plugins/events_test.cljs

@@ -0,0 +1,66 @@
+;; This Source Code Form is subject to the terms of the Mozilla Public
+;; License, v. 2.0. If a copy of the MPL was not distributed with this
+;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+;;
+;; Copyright (c) KALEIDOS INC Sucursal en España SL
+
+(ns frontend-tests.plugins.events-test
+  "Unit tests for app.plugins.events, the plugin state-change listeners."
+  (:require
+   [app.common.uuid :as uuid]
+   [app.main.store :as st]
+   [app.plugins.events :as events]
+   [cljs.test :as t :include-macros true]))
+
+(def ^:private plugin-id (str (uuid/next)))
+
+;; A listener type that always fails. The dispatch value is namespaced so
+;; product code never reaches it.
+(defmethod events/handle-state-change "test/always-throws"
+  [_ _ _ _ _]
+  (throw (js/Error. "listener boom")))
+
+(defn- touch-state!
+  "Applies a throwaway state change to fire the store watches."
+  []
+  (swap! st/state assoc ::probe (uuid/next)))
+
+(t/deftest shapechange-listener-with-blank-shape-id-does-not-raise
+  ;; The listener watches the store, so it runs on every state change. A
+  ;; raise there reaches the global error handler, whose error toast
+  ;; changes the state again and re-enters the listener.
+  (let [key (events/add-listener "shapechange" plugin-id (fn [_]) #js {"shapeId" ""})]
+    (try
+      (t/is (some? (touch-state!)))
+      (finally
+        (events/remove-listener key)
+        (swap! st/state dissoc ::probe)))))
+
+(t/deftest malformed-shape-id-is-rejected-at-registration
+  (t/testing "the plugin sees the failure when it registers the listener"
+    (t/is (thrown? js/Error
+                   (events/add-listener "shapechange" plugin-id (fn [_]) #js {"shapeId" "not-a-uuid"}))))
+
+  (t/testing "no watch survives a rejected registration"
+    (let [seen (atom 0)]
+      (add-watch st/state ::probe-watch (fn [_ _ _ _] (swap! seen inc)))
+      (try
+        (touch-state!)
+        (t/is (= 1 @seen))
+        (finally
+          (remove-watch st/state ::probe-watch)
+          (swap! st/state dissoc ::probe))))))
+
+(t/deftest failing-listener-does-not-starve-other-watches
+  ;; Watches are notified by a plain iteration: a listener that raises
+  ;; aborts the loop, and the watches registered after it miss the change.
+  (let [seen (atom 0)
+        key  (events/add-listener "test/always-throws" plugin-id (fn [_]) nil)]
+    (add-watch st/state ::probe-watch (fn [_ _ _ _] (swap! seen inc)))
+    (try
+      (t/is (some? (touch-state!)))
+      (t/is (= 1 @seen))
+      (finally
+        (remove-watch st/state ::probe-watch)
+        (events/remove-listener key)
+        (swap! st/state dissoc ::probe)))))

frontend/test/frontend_tests/plugins/parser_test.cljs

@@ -173,3 +173,23 @@
                (:animation result)))
       (t/is (true? (sm/validate ctsi/schema:interaction result))))))
 
+
+(t/deftest test-parse-id-treats-blank-as-absent
+  ;; `""` is truthy in ClojureScript, so an unguarded blank id reaches
+  ;; `uuid/parse`. Plugins pass ids straight from JS, where an absent
+  ;; value is routinely an empty string.
+  (t/testing "nil is absent"
+    (t/is (nil? (parser/parse-id nil))))
+
+  (t/testing "an empty string is absent"
+    (t/is (nil? (parser/parse-id ""))))
+
+  (t/testing "a whitespace-only string is absent"
+    (t/is (nil? (parser/parse-id "   "))))
+
+  (t/testing "a valid uuid string is parsed"
+    (let [id (uuid/next)]
+      (t/is (= id (parser/parse-id (str id))))))
+
+  (t/testing "a malformed id raises, so plugin bugs stay visible"
+    (t/is (thrown? js/Error (parser/parse-id "not-a-uuid")))))

frontend/test/frontend_tests/runner.cljs

@@ -53,6 +53,7 @@
    [frontend-tests.main.refs-test]
    [frontend-tests.plugins.comments-test]
    [frontend-tests.plugins.context-shapes-test]
+   [frontend-tests.plugins.events-test]
    [frontend-tests.plugins.file-test]
    [frontend-tests.plugins.flex-test]
    [frontend-tests.plugins.format-test]
@@ -167,6 +168,7 @@
    'frontend-tests.logic.wasm-pixel-snap-test
    'frontend-tests.plugins.comments-test
    'frontend-tests.plugins.context-shapes-test
+   'frontend-tests.plugins.events-test
    'frontend-tests.plugins.file-test
    'frontend-tests.plugins.flex-test
    'frontend-tests.plugins.format-test

plugins/apps/plugin-api-test-suite/src/tests/events.test.ts

@@ -67,4 +67,33 @@ describe('Events', () => {
 
     expect(count).toBe(0);
   });
+
+  test('a blank shapeId leaves the other listeners working', async (ctx) => {
+    const rect = ctx.penpot.createRectangle();
+    ctx.board.appendChild(rect);
+
+    // Listeners are store watches notified by a plain iteration, so one that
+    // raises aborts the loop and the listeners after it miss the change.
+    const blankListenerId = ctx.penpot.on('shapechange', () => {}, {
+      shapeId: '',
+    });
+
+    let received: string[] | null = null;
+    const listenerId = ctx.penpot.on('selectionchange', (ids) => {
+      received = ids;
+    });
+
+    ctx.penpot.selection = [rect];
+    await waitFor(() => received !== null);
+    ctx.penpot.off(listenerId);
+    ctx.penpot.off(blankListenerId);
+
+    expect(received).not.toBeNull();
+  });
+
+  test('a malformed shapeId is rejected when the listener is registered', (ctx) => {
+    expect(() =>
+      ctx.penpot.on('shapechange', () => {}, { shapeId: 'not-a-uuid' }),
+    ).toThrow();
+  });
 });