From b5274a44766d095247037be18c1d1918ac67ddeb Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Sep 2026 11:53:23 +0200 Subject: [PATCH 1/4] :bug: Bind MCP PluginBridge WebSocket to configured host (#11605) The plugin WebSocket bridge ignored PENPOT_MCP_SERVER_HOST and bound all interfaces, exposing unauthenticated task dispatch to the network in single-user mode. Pass mcpServer.host into WebSocketServer, mirroring the ReplServer fix, so the bridge binds localhost by default and 0.0.0.0 only on explicit opt-in. Closes #11603. AI-assisted-by: muse-spark-1.3-contributor --- mcp/packages/server/src/PluginBridge.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index 6a1e07f9c5..305f5dc35e 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -94,7 +94,7 @@ export class PluginBridge { private readonly taskTimeoutSecs: number, private readonly redisBridge?: RedisBridge ) { - this.wsServer = new WebSocketServer({ port: port }); + this.wsServer = new WebSocketServer({ port: port, host: mcpServer.host }); this.setupWebSocketHandlers(); } From b9bb62b49753e3abb5e0294df1cb0c0399776ad7 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Sep 2026 12:17:59 +0200 Subject: [PATCH 2/4] :bug: Block MCP REPL server in multi-user mode and decouple its host (#11632) The unauthenticated REPL server started in --multi-user mode whenever DEVENV or REPL_ENABLE was set, unlike developer tools which are already blocked there. Gate its creation behind shouldStartReplServer, mirroring the developer tools policy, so it never starts with --multi-user. Give it a dedicated PENPOT_MCP_REPL_HOST knob defaulting to localhost so it no longer inherits the main server 0.0.0.0 bind in Docker, keep the devenv script on 0.0.0.0, and document and test the new behavior. Closes #11631 AI-assisted-by: muse-spark-1.3-contributor --- mcp/README.md | 3 +- .../server/src/PenpotMcpServer.test.ts | 103 +++++++++++++++++- mcp/packages/server/src/PenpotMcpServer.ts | 25 ++++- mcp/scripts/start-mcp-devenv | 2 +- 4 files changed, 123 insertions(+), 10 deletions(-) diff --git a/mcp/README.md b/mcp/README.md index 702eec3d60..3189a4dea3 100644 --- a/mcp/README.md +++ b/mcp/README.md @@ -265,7 +265,8 @@ The Penpot MCP server can be configured using environment variables. | `PENPOT_MCP_SERVER_PORT` | Port for the HTTP/SSE server | `4401` | | `PENPOT_MCP_WEBSOCKET_PORT` | Port for the WebSocket server (plugin connection) | `4402` | | `PENPOT_MCP_REPL_PORT` | Port for the REPL server (development/debugging) | `4403` | -| `PENPOT_MCP_REPL_ENABLE` | Explicitly enable/disable the REPL server. Set to `true` to enable. When unset, defaults to the value of `PENPOT_MCP_DEVENV`. | (unset) | +| `PENPOT_MCP_REPL_HOST` | Address on which the REPL server listens (binds to) | `localhost` | +| `PENPOT_MCP_REPL_ENABLE` | Explicitly enable/disable the REPL server. Set to `true` to enable. When unset, defaults to the value of `PENPOT_MCP_DEVENV`. The REPL server never starts in multi-user mode. | (unset) | | `PENPOT_MCP_REMOTE_MODE` | Enable remote mode (disables file system access). Set to `true` to enable. | `false` | | `PENPOT_MCP_DEVENV` | Enable Penpot development environment tools in local single-user mode. Set to `true` to enable. | `false` | | `PENPOT_MCP_TOOL_TIMEOUT_S` | Timeout, in seconds, for tool calls dispatched to the Penpot plugin | `120` | diff --git a/mcp/packages/server/src/PenpotMcpServer.test.ts b/mcp/packages/server/src/PenpotMcpServer.test.ts index 68665359f5..2d35e70589 100644 --- a/mcp/packages/server/src/PenpotMcpServer.test.ts +++ b/mcp/packages/server/src/PenpotMcpServer.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { PenpotMcpServer, shouldRegisterDeveloperTools } from "./PenpotMcpServer"; +import { PenpotMcpServer, shouldRegisterDeveloperTools, shouldStartReplServer } from "./PenpotMcpServer"; test("registers developer tools in local devenv mode", () => { assert.equal(shouldRegisterDeveloperTools(true, false), true); @@ -14,6 +14,22 @@ test("does not register developer tools when devenv mode is disabled", () => { assert.equal(shouldRegisterDeveloperTools(false, false), false); }); +test("starts REPL server in single-user mode when enabled", () => { + assert.equal(shouldStartReplServer(true, false), true); +}); + +test("does not start REPL server in multi-user mode even when enabled", () => { + assert.equal(shouldStartReplServer(true, true), false); +}); + +test("does not start REPL server when disabled in single-user mode", () => { + assert.equal(shouldStartReplServer(false, false), false); +}); + +test("does not start REPL server when disabled in multi-user mode", () => { + assert.equal(shouldStartReplServer(false, true), false); +}); + // ── Pure function tests ──────────────────────────────────────── test("isDevEnvEnabled returns false when PENPOT_MCP_DEVENV is not set", () => { @@ -125,6 +141,79 @@ test("constructor does not create ReplServer when PENPOT_MCP_REPL_ENABLE is 'fal } }); +test("constructor does not create ReplServer in multi-user mode even with DEVENV", async () => { + const prevDevEnv = process.env.PENPOT_MCP_DEVENV; + const prevReplEnable = process.env.PENPOT_MCP_REPL_ENABLE; + const prevPorts = setUniqueEnv(); + process.env.PENPOT_MCP_DEVENV = "true"; + delete process.env.PENPOT_MCP_REPL_ENABLE; + let server: PenpotMcpServer | undefined; + try { + server = new PenpotMcpServer(true); + assert.equal(server.hasReplServer(), false); + } finally { + await server?.stop(); + restoreEnv(prevDevEnv, prevPorts); + restoreOrDelete("PENPOT_MCP_REPL_ENABLE", prevReplEnable); + } +}); + +test("constructor does not create ReplServer in multi-user mode even with explicit REPL_ENABLE", async () => { + const prevDevEnv = process.env.PENPOT_MCP_DEVENV; + const prevReplEnable = process.env.PENPOT_MCP_REPL_ENABLE; + const prevPorts = setUniqueEnv(); + delete process.env.PENPOT_MCP_DEVENV; + process.env.PENPOT_MCP_REPL_ENABLE = "true"; + let server: PenpotMcpServer | undefined; + try { + server = new PenpotMcpServer(true); + assert.equal(server.hasReplServer(), false); + } finally { + await server?.stop(); + restoreEnv(prevDevEnv, prevPorts); + restoreOrDelete("PENPOT_MCP_REPL_ENABLE", prevReplEnable); + } +}); + +test("replHost defaults to localhost and ignores SERVER_HOST", async () => { + const prevDevEnv = process.env.PENPOT_MCP_DEVENV; + const prevServerHost = process.env.PENPOT_MCP_SERVER_HOST; + const prevReplHost = process.env.PENPOT_MCP_REPL_HOST; + const prevPorts = setUniqueEnv(); + process.env.PENPOT_MCP_DEVENV = "true"; + process.env.PENPOT_MCP_SERVER_HOST = "0.0.0.0"; + delete process.env.PENPOT_MCP_REPL_HOST; + let server: PenpotMcpServer | undefined; + try { + server = new PenpotMcpServer(false); + assert.equal(server.hasReplServer(), true); + assert.equal(server.replHost, "localhost"); + } finally { + await server?.stop(); + restoreEnv(prevDevEnv, prevPorts); + restoreOrDelete("PENPOT_MCP_SERVER_HOST", prevServerHost); + restoreOrDelete("PENPOT_MCP_REPL_HOST", prevReplHost); + } +}); + +test("replHost respects PENPOT_MCP_REPL_HOST", async () => { + const prevDevEnv = process.env.PENPOT_MCP_DEVENV; + const prevReplHost = process.env.PENPOT_MCP_REPL_HOST; + const prevPorts = setUniqueEnv(); + process.env.PENPOT_MCP_DEVENV = "true"; + process.env.PENPOT_MCP_REPL_HOST = "0.0.0.0"; + let server: PenpotMcpServer | undefined; + try { + server = new PenpotMcpServer(false); + assert.equal(server.hasReplServer(), true); + assert.equal(server.replHost, "0.0.0.0"); + } finally { + await server?.stop(); + restoreEnv(prevDevEnv, prevPorts); + restoreOrDelete("PENPOT_MCP_REPL_HOST", prevReplHost); + } +}); + // ── Helpers ──────────────────────────────────────────────────── function setUniqueEnv() { @@ -132,15 +221,22 @@ function setUniqueEnv() { const prevServer = process.env.PENPOT_MCP_SERVER_PORT; const prevWs = process.env.PENPOT_MCP_WEBSOCKET_PORT; const prevRepl = process.env.PENPOT_MCP_REPL_PORT; + const prevReplHost = process.env.PENPOT_MCP_REPL_HOST; process.env.PENPOT_MCP_SERVER_PORT = String(ports.server); process.env.PENPOT_MCP_WEBSOCKET_PORT = String(ports.ws); process.env.PENPOT_MCP_REPL_PORT = String(ports.repl); - return { prevServer, prevWs, prevRepl }; + delete process.env.PENPOT_MCP_REPL_HOST; + return { prevServer, prevWs, prevRepl, prevReplHost }; } function restoreEnv( devEnv: string | undefined, - ports: { prevServer: string | undefined; prevWs: string | undefined; prevRepl: string | undefined } + ports: { + prevServer: string | undefined; + prevWs: string | undefined; + prevRepl: string | undefined; + prevReplHost: string | undefined; + } ) { if (devEnv !== undefined) { process.env.PENPOT_MCP_DEVENV = devEnv; @@ -150,6 +246,7 @@ function restoreEnv( restoreOrDelete("PENPOT_MCP_SERVER_PORT", ports.prevServer); restoreOrDelete("PENPOT_MCP_WEBSOCKET_PORT", ports.prevWs); restoreOrDelete("PENPOT_MCP_REPL_PORT", ports.prevRepl); + restoreOrDelete("PENPOT_MCP_REPL_HOST", ports.prevReplHost); } function restoreOrDelete(key: string, value: string | undefined) { diff --git a/mcp/packages/server/src/PenpotMcpServer.ts b/mcp/packages/server/src/PenpotMcpServer.ts index c620aca3fc..4691c0541d 100644 --- a/mcp/packages/server/src/PenpotMcpServer.ts +++ b/mcp/packages/server/src/PenpotMcpServer.ts @@ -57,6 +57,16 @@ export function shouldRegisterDeveloperTools(isDevEnv: boolean, isMultiUserMode: return isDevEnv && !isMultiUserMode; } +/** + * Indicates whether the REPL server may be started for the current server mode. + * + * The REPL server never starts in multi-user mode, even when explicitly + * enabled, mirroring the developer tools policy. + */ +export function shouldStartReplServer(isReplEnabled: boolean, isMultiUserMode: boolean): boolean { + return isReplEnabled && !isMultiUserMode; +} + export class PenpotMcpServer { /** * Timeout, in minutes, for idle sessions (Streamable HTTP and SSE) before they are automatically closed and removed. @@ -133,6 +143,7 @@ export class PenpotMcpServer { public readonly host: string; public readonly port: number; public readonly webSocketPort: number; + public readonly replHost: string; public readonly replPort: number; private sessionTimeoutInterval: ReturnType | undefined; @@ -156,6 +167,7 @@ export class PenpotMcpServer { this.host = process.env.PENPOT_MCP_SERVER_HOST ?? "localhost"; this.port = parseInt(process.env.PENPOT_MCP_SERVER_PORT ?? "4401", 10); this.webSocketPort = parseInt(process.env.PENPOT_MCP_WEBSOCKET_PORT ?? "4402", 10); + this.replHost = process.env.PENPOT_MCP_REPL_HOST ?? "localhost"; this.replPort = parseInt(process.env.PENPOT_MCP_REPL_PORT ?? "4403", 10); this.tenant = process.env.PENPOT_TENANT ?? "default"; const toolTimeoutSecs = parseInt(process.env.PENPOT_MCP_TOOL_TIMEOUT_S ?? "120", 10); @@ -181,8 +193,8 @@ export class PenpotMcpServer { this.pluginBridge = new PluginBridge(this, this.webSocketPort, toolTimeoutSecs, this.redisBridge); - if (PenpotMcpServer.isReplEnabled(process.env)) { - this.replServer = new ReplServer(this.pluginBridge, this.replPort, this.host); + if (shouldStartReplServer(PenpotMcpServer.isReplEnabled(process.env), this.isMultiUserMode())) { + this.replServer = new ReplServer(this.pluginBridge, this.replPort, this.replHost); } else { this.replServer = null; } @@ -232,9 +244,10 @@ export class PenpotMcpServer { /** * Indicates whether the REPL server was created. * - * The REPL server is created when {@link isReplEnabled} returns true, - * which means either ``PENPOT_MCP_REPL_ENABLE=true`` or, when that - * variable is unset, ``PENPOT_MCP_DEVENV=true``. + * The REPL server is created when {@link isReplEnabled} returns true and + * the server is not running in multi-user mode, which means either + * ``PENPOT_MCP_REPL_ENABLE=true`` or, when that variable is unset, + * ``PENPOT_MCP_DEVENV=true``, in single-user mode. */ public hasReplServer(): boolean { return this.replServer !== null; @@ -471,6 +484,8 @@ export class PenpotMcpServer { // start the REPL server (devenv only) and session timeout checker if (this.replServer) { await this.replServer.start(); + } else if (this.isMultiUserMode()) { + this.logger.info("REPL server disabled in multi-user mode (never started with --multi-user)"); } else { this.logger.info( "REPL server disabled (set PENPOT_MCP_REPL_ENABLE=true or PENPOT_MCP_DEVENV=true to enable)" diff --git a/mcp/scripts/start-mcp-devenv b/mcp/scripts/start-mcp-devenv index 62e957a9eb..9098a8b254 100755 --- a/mcp/scripts/start-mcp-devenv +++ b/mcp/scripts/start-mcp-devenv @@ -3,4 +3,4 @@ # This starts the MCP server in a configuration for Penpot development # (assuming devenv) -PENPOT_MCP_SERVER_HOST=0.0.0.0 PENPOT_MCP_REMOTE_MODE=true PENPOT_MCP_DEVENV=true pnpm run bootstrap +PENPOT_MCP_SERVER_HOST=0.0.0.0 PENPOT_MCP_REPL_HOST=0.0.0.0 PENPOT_MCP_REMOTE_MODE=true PENPOT_MCP_DEVENV=true pnpm run bootstrap From 31b73460c32e9f9fe4aa5d4346346418694e81dc Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Tue, 22 Sep 2026 12:56:01 +0200 Subject: [PATCH 3/4] :bug: Fix problem with viewer url (#11803) --- frontend/src/app/main/data/viewer.cljs | 14 ++++- .../test/frontend_tests/data/viewer_test.cljs | 55 +++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/main/data/viewer.cljs b/frontend/src/app/main/data/viewer.cljs index cbf63f4773..418a34ae87 100644 --- a/frontend/src/app/main/data/viewer.cljs +++ b/frontend/src/app/main/data/viewer.cljs @@ -367,9 +367,17 @@ ptk/WatchEvent (watch [_ state _] (let [zoom-type (get-in state [:viewer-local :zoom-type]) - params (rt/get-params state)] - - (rx/of (rt/nav :viewer (assoc params :zoom zoom-type))))))) + params (rt/get-params state) + current (rt/get-query-param params :zoom) + expected (some-> zoom-type name)] + ;; Zoom is view state: mirror it into the URL, replacing the history + ;; entry, only when the query string does not already describe it. + (when (not= current expected) + (rx/of (rt/nav :viewer + (if (some? zoom-type) + (assoc params :zoom zoom-type) + (dissoc params :zoom)) + {::rt/replace true}))))))) (def increase-zoom (ptk/reify ::increase-zoom diff --git a/frontend/test/frontend_tests/data/viewer_test.cljs b/frontend/test/frontend_tests/data/viewer_test.cljs index 5125ca9674..0aea84ddba 100644 --- a/frontend/test/frontend_tests/data/viewer_test.cljs +++ b/frontend/test/frontend_tests/data/viewer_test.cljs @@ -8,6 +8,8 @@ (:require [app.common.uuid :as uuid] [app.main.data.viewer :as dv] + [app.main.router :as rt] + [beicon.v2.core :as rx] [cljs.test :as t] [potok.v2.core :as ptk])) @@ -67,3 +69,56 @@ result (ptk/update dv/zoom-to-fill state)] (t/is (= (get-in result [:viewer-local :zoom-type]) :fill)) (t/is (number? (get-in result [:viewer-local :zoom])))))) + +(defn- watch-events + "Collect the events an event's watch emits synchronously." + [event state] + (let [out (atom [])] + (some-> (ptk/watch event state nil) + (rx/subscribe #(swap! out conj %))) + @out)) + +(defn- zoom-state + "Build a viewer state with the given `:zoom` query param and zoom type." + [zoom-param zoom-type] + {:route {:params {:query (cond-> {:page-id (str page-id) :index "0"} + (some? zoom-param) + (assoc :zoom zoom-param))}} + :viewer-local (cond-> {} + (some? zoom-type) + (assoc :zoom-type zoom-type))}) + +(t/deftest update-zoom-querystring-does-not-navigate-when-url-already-matches + (t/testing "zoom type already described by the query string" + (t/is (empty? (watch-events dv/update-zoom-querystring + (zoom-state "fit" :fit))))) + + (t/testing "no zoom type and no zoom query param" + (t/is (empty? (watch-events dv/update-zoom-querystring + (zoom-state nil nil)))))) + +(t/deftest update-zoom-querystring-navigates-when-zoom-changes + (t/testing "zoom type differs from the query string" + (let [events (watch-events dv/update-zoom-querystring + (zoom-state "fit" :fill)) + {:keys [id params options]} (some-> (first events) deref)] + (t/is (= 1 (count events))) + (t/is (= :viewer id)) + (t/is (= :fill (:zoom params))) + (t/is (true? (::rt/replace options))))) + + (t/testing "zoom query param absent, other params preserved" + (let [events (watch-events dv/update-zoom-querystring + (zoom-state nil :fit)) + {:keys [params]} (some-> (first events) deref)] + (t/is (= 1 (count events))) + (t/is (= :fit (:zoom params))) + (t/is (= (str page-id) (:page-id params))) + (t/is (= "0" (:index params))))) + + (t/testing "zoom type cleared drops the query param" + (let [events (watch-events dv/update-zoom-querystring + (zoom-state "fit" nil)) + {:keys [params]} (some-> (first events) deref)] + (t/is (= 1 (count events))) + (t/is (not (contains? params :zoom)))))) From efb10c82a85304995c2eb0530604827a6c80667e Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Tue, 22 Sep 2026 12:56:35 +0200 Subject: [PATCH 4/4] :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);