🐛 Fix silent fail in plugin API interactions (#11781)

frontend/src/app/main/data/workspace/interactions.cljs

@@ -210,7 +210,7 @@
      ptk/WatchEvent
      (watch [_ _ _]
        (let [interactions (ctsi/update-interaction (:interactions shape) index update-fn)
-             interaction (nth interactions index)]
+             interaction (get interactions index)]
          (rx/of
           (dwsh/update-shapes
            [(:id shape)]

frontend/src/app/plugins/shape.cljs

@@ -105,7 +105,8 @@
        :get #(-> % u/proxy->interaction :event-type format/format-key)
        :set
        (fn [_ value]
-         (let [value (parser/parse-keyword value)]
+         (let [value (parser/parse-keyword value)
+               index (locate-index)]
            (cond
              (not (contains? ctsi/event-types value))
              (u/not-valid plugin-id :trigger value)
@@ -113,11 +114,14 @@
              (not (r/check-permission plugin-id "content:write"))
              (u/not-valid plugin-id :trigger "Plugin doesn't have 'content:write' permission")
 
+             (nil? index)
+             (u/not-valid plugin-id :trigger "The interaction is not part of the shape anymore")
+
              :else
              (do
                (st/emit! (dwi/update-interaction
                           (u/locate-shape file-id page-id shape-id)
-                          (locate-index)
+                          index
                           #(assoc % :event-type value)
                           {:page-id page-id}))
                (swap! current assoc :event-type value)))))}
@@ -127,21 +131,25 @@
        :get #(-> % u/proxy->interaction :delay)
        :set
        (fn [_ value]
-         (cond
+         (let [index (locate-index)]
-           (or (not (sm/valid-safe-int? value)) (neg? value))
+           (cond
-           (u/not-valid plugin-id :delay 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"))
+             (not (r/check-permission plugin-id "content:write"))
-           (u/not-valid plugin-id :delay "Plugin doesn't have 'content:write' permission")
+             (u/not-valid plugin-id :delay "Plugin doesn't have 'content:write' permission")
 
-           :else
+             (nil? index)
-           (do
+             (u/not-valid plugin-id :delay "The interaction is not part of the shape anymore")
-             (st/emit! (dwi/update-interaction
+
-                        (u/locate-shape file-id page-id shape-id)
+             :else
-                        (locate-index)
+             (do
-                        #(assoc % :delay value)
+               (st/emit! (dwi/update-interaction
-                        {:page-id page-id}))
+                          (u/locate-shape file-id page-id shape-id)
-             (swap! current assoc :delay value))))}
+                          index
+                          #(assoc % :delay value)
+                          {:page-id page-id}))
+               (swap! current assoc :delay value)))))}
 
       :action
       {:this true
@@ -149,21 +157,27 @@
        :set
        (fn [self value]
          (let [params (parser/parse-action value)
+               index  (locate-index)
                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 :action "Plugin doesn't have 'content:write' permission")
 
+             ;; Precedes the schema check, which sees only the partial map that
+             ;; patching a missing interaction produces.
+             (nil? index)
+             (u/not-valid plugin-id :action "The interaction is not part of the shape anymore")
+
+             (not (sm/validate ctsi/schema:interaction interaction))
+             (u/not-valid plugin-id :action interaction)
+
              :else
              (do
                (st/emit! (dwi/update-interaction
                           (u/locate-shape file-id page-id shape-id)
-                          (locate-index)
+                          index
                           #(d/patch-object % params)
                           {:page-id page-id}))
                (reset! current interaction)))))}

plugins/CHANGELOG.md

@@ -10,6 +10,7 @@
 
 - **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**: Writing `trigger`, `delay` or `action` on an interaction the shape no longer has now raises a validation error. The write used to be sent to the workspace with no position to apply it at, where it failed out of the plugin's reach: nothing was written and nothing was reported.
 - **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/README.md

@@ -69,6 +69,27 @@ E2E_LOGIN_EMAIL=… E2E_LOGIN_PASSWORD=… \
 - `PRINT_UNCOVERED=1` dumps the uncovered targets per interface; `PRINT_STATIC=1`
   dumps the statically-covered ones (see [Coverage](#how-coverage-works-and-how-to-write-tests-that-move-it)).
 
+### Errors the app reports on its own
+
+An API call can leave the plugin happy and still break Penpot: an exception
+raised inside an event handler is caught by the store, which reports it and
+carries on, so nothing is thrown back across the sandbox. The CI runner watches
+the page console for the prefixes Penpot's error handler prints (`Internal
+Error`, `Unexpected Error`, `Assertion Error`, `Uncaught Exception`, `Uncaught
+Rejection`) and for uncaught page errors, and fails the test that was running.
+
+Two consequences when writing a test for such a case:
+
+- Await the API (`await ctx.penpot.waitForLayoutUpdate()`) after the operation,
+  so the message reaches the console before the test ends and is attributed to
+  it rather than to the next one.
+- This runs in the CI runner only. The plugin UI cannot read the page console,
+  so the same test shows green there — check it with `test:ci` or
+  `test:ci:mocked`.
+
+`Plugin Error` and `Network Error` are not watched: tests provoke both on
+purpose.
+
 CI entry points reuse the exact same test files (`src/ci/headless.ts` discovers
 them the same way the plugin does).
 

plugins/apps/plugin-api-test-suite/ci/run-ci.ts

@@ -34,6 +34,13 @@ const frontendDir = resolve(repoRoot, 'frontend');
 const staticRoot = resolve(frontendDir, 'resources/public');
 const e2eDataDir = resolve(frontendDir, 'playwright/data');
 
+// Console prefixes Penpot's error handler prints for failures the app did not
+// expect (`frontend/src/app/main/errors.cljs`). The store swallows these, so
+// the console is the only place a test can observe them. "Plugin Error" and
+// "Network Error" are left out: tests provoke both on purpose.
+const APP_ERROR_RE =
+  /^(Internal Error|Unexpected Error|Assertion Error|Uncaught Exception|Uncaught Rejection):/;
+
 const MOCKED = !!process.env['MOCK_BACKEND'];
 const MOCK_BASE_URL = 'http://localhost:3000';
 const apiUrl = MOCKED
@@ -393,13 +400,36 @@ async function main() {
   let fatal: string | null = null;
 
   console.log('\nRunning tests:');
+  // Errors the app reported since the previous test result.
+  let appErrors: string[] = [];
+  const takeAppErrors = (): string => {
+    const detail = appErrors.join('; ');
+    appErrors = [];
+    return detail;
+  };
+
   const done = new Promise<void>((resolvePromise) => {
+    page.on('pageerror', (err) => {
+      appErrors.push(`Uncaught ${err.message}`);
+    });
     page.on('console', (msg) => {
       const text = msg.text();
+      if (APP_ERROR_RE.test(text)) {
+        appErrors.push(text.split('\n')[0]!.trim());
+      }
       if (text.startsWith('__TEST_RESULT__ ')) {
         const result: TestResult = JSON.parse(
           text.slice('__TEST_RESULT__ '.length),
         );
+        // Errors buffered so far belong to the test this result closes.
+        const reported = takeAppErrors();
+        if (reported) {
+          result.error =
+            result.status === 'fail' && result.error
+              ? `${result.error} — Penpot also reported: ${reported}`
+              : `Penpot reported an error during the test: ${reported}`;
+          result.status = 'fail';
+        }
         results.push(result);
         // Print each result as it streams in so the run shows live progress
         // instead of staying silent until it finishes.

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

@@ -413,6 +413,28 @@ describe('Interactions', () => {
     expect(r.interactions.map((i) => i.delay)).toEqual([null, 500]);
   });
 
+  // A proxy whose own interaction is gone resolves to no position, so the write
+  // has nothing to address and the API rejects it.
+  test('a write through a removed interaction is rejected', async (ctx) => {
+    const r = rect(ctx);
+    r.addInteraction('click', {
+      type: 'open-url',
+      url: 'https://example.com',
+    });
+    await ctx.penpot.waitForLayoutUpdate();
+    const [only] = r.interactions;
+
+    only.remove();
+    await ctx.penpot.waitForLayoutUpdate();
+    expect(r.interactions).toHaveLength(0);
+
+    expect(() => {
+      only.delay = 500;
+    }).toThrow('The interaction is not part of the shape anymore');
+    await ctx.penpot.waitForLayoutUpdate();
+    expect(r.interactions).toHaveLength(0);
+  });
+
   test('interaction trigger can be changed', (ctx) => {
     const dest = board(ctx);
     const r = rect(ctx);