diff --git a/frontend/src/app/plugins/events.cljs b/frontend/src/app/plugins/events.cljs index 7b52ba65f1..884a1e0bac 100644 --- a/frontend/src/app/plugins/events.cljs +++ b/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)) diff --git a/frontend/src/app/plugins/parser.cljs b/frontend/src/app/plugins/parser.cljs index 5923289407..a13dd5e69d 100644 --- a/frontend/src/app/plugins/parser.cljs +++ b/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] diff --git a/frontend/test/frontend_tests/plugins/events_test.cljs b/frontend/test/frontend_tests/plugins/events_test.cljs new file mode 100644 index 0000000000..b32a9914c7 --- /dev/null +++ b/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))))) diff --git a/frontend/test/frontend_tests/plugins/parser_test.cljs b/frontend/test/frontend_tests/plugins/parser_test.cljs index 62f5bb26ed..d39c37851c 100644 --- a/frontend/test/frontend_tests/plugins/parser_test.cljs +++ b/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"))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 93dbcdad06..158a8fbc83 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/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 diff --git a/plugins/apps/plugin-api-test-suite/src/tests/events.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/events.test.ts index 23203c88f9..042d6c84d5 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/events.test.ts +++ b/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(); + }); });