From efb10c82a85304995c2eb0530604827a6c80667e Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Tue, 22 Sep 2026 12:56:35 +0200 Subject: [PATCH] :bug: Fix silent fail in plugin API interactions (#11781) --- .../app/main/data/workspace/interactions.cljs | 2 +- frontend/src/app/plugins/shape.cljs | 52 ++++++++++++------- plugins/CHANGELOG.md | 1 + plugins/apps/plugin-api-test-suite/README.md | 21 ++++++++ .../apps/plugin-api-test-suite/ci/run-ci.ts | 30 +++++++++++ .../src/tests/interactions.test.ts | 22 ++++++++ 6 files changed, 108 insertions(+), 20 deletions(-) diff --git a/frontend/src/app/main/data/workspace/interactions.cljs b/frontend/src/app/main/data/workspace/interactions.cljs index 673697a74d..5789c62e9a 100644 --- a/frontend/src/app/main/data/workspace/interactions.cljs +++ b/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)] diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 81bcf89628..e56517492f 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/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 - (or (not (sm/valid-safe-int? value)) (neg? value)) - (u/not-valid plugin-id :delay value) + (let [index (locate-index)] + (cond + (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 :delay "Plugin doesn't have 'content:write' permission") + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :delay "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 % :delay value) - {:page-id page-id})) - (swap! current assoc :delay value))))} + (nil? index) + (u/not-valid plugin-id :delay "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) + 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)))))} diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 7e0cd7491b..95af043ddc 100644 --- a/plugins/CHANGELOG.md +++ b/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. diff --git a/plugins/apps/plugin-api-test-suite/README.md b/plugins/apps/plugin-api-test-suite/README.md index af01f7e4ca..5ea524b484 100644 --- a/plugins/apps/plugin-api-test-suite/README.md +++ b/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). diff --git a/plugins/apps/plugin-api-test-suite/ci/run-ci.ts b/plugins/apps/plugin-api-test-suite/ci/run-ci.ts index 801d62779d..559e6f0aea 100644 --- a/plugins/apps/plugin-api-test-suite/ci/run-ci.ts +++ b/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((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. 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 6f01f8ef0f..a7fcc88051 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 @@ -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);