From 642f35cf905b6f6b6c362a22dd04de980a9e0bb9 Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Tue, 3 Nov 2026 11:22:30 +0000 Subject: [PATCH] :bug: Let plugins write in Inspect mode and stop them as viewers Plugin writes failed whenever the workspace read-only flag was set. The Inspect tab sets it too, although it only hides the editing tools, so an MCP agent could not edit while the user was inspecting. A user who became a viewer had the flag cleared, so plugin writes did not throw and silently did nothing. Writes now fail on what makes them unsafe, not on the flag: failing saves, a user who cannot edit, a version preview or a lost render context. The error says which one, and the MCP instructions tell the agent which of them pass by waiting. --- .../frontend/plugin-api-to-cljs-binding.md | 2 +- frontend/src/app/plugins/register.cljs | 35 +++++++++++++----- .../plugins/read_only_test.cljs | 36 ++++++++++++++----- .../plugins/shape_bugfixes_test.cljs | 3 +- .../server/data/initial_instructions.md | 4 ++- plugins/CHANGELOG.md | 2 +- 6 files changed, 61 insertions(+), 21 deletions(-) diff --git a/.serena/memories/frontend/plugin-api-to-cljs-binding.md b/.serena/memories/frontend/plugin-api-to-cljs-binding.md index 0338eca277..080c4b009c 100644 --- a/.serena/memories/frontend/plugin-api-to-cljs-binding.md +++ b/.serena/memories/frontend/plugin-api-to-cljs-binding.md @@ -24,7 +24,7 @@ - Public Plugin API objects are lightweight handles, not durable snapshots. Most getters locate fresh state from `app.main.store/state` using hidden `$id`, `$file`, `$page`, etc. - `not-valid` logs by default but throws when the plugin flag `throwValidationErrors` is enabled. The MCP execute-code handler deliberately enables that flag while running code. - A write to a removed shape always throws `Shape not found`, whatever that flag: setters through the shape proxy's `:on-set u/check-shape-writable` (runs outside the error handling), and methods that must not vanish silently (`resize`, `rotate`) call `u/check-shape-located` first. Methods run inside `:on-error (u/handle-error ...)`, which logs instead of throwing when the flag is off, so it rethrows errors whose `ex-data :type` is `::u/shape-not-found` or `::r/read-only`. -- A write in a read-only workspace (`[:workspace-global :read-only?]`: save-failure lock, user without edit rights, version preview) always throws, whatever that flag. `r/check-permission` with `content:write`/`library:write` calls `r/check-writable` first, which covers most writers; writers without a permission check (most top-level context methods in `api.cljs`: `create*`, `group`, `align*`, …; z-order methods; text range `applyTypography`; ruler guide `remove`) call `r/check-writable` themselves, and shape setters get it through `u/check-shape-writable`. A new writer without a write-permission check must call it too. `r/has-permission?` is the check without it, used by the undo blocks so a block opened before a lock still closes. Comments (`comment:write`) stay writable: they do not go through file saves. An async writer calls it again after each server await, before the next server write or commit (`rx/tap` on the response stream; `dwm/create-svg-shape-with-images` takes it as `:check-writable`), so a lock that lands mid-way rejects the promise with the read-only error. `r/has-permission?` is the check without it, used by the undo blocks so a block opened before a lock still closes. Comments (`comment:write`) stay writable: they do not go through file saves. +- A write while the workspace cannot take it (`r/read-only-reason`: save-failure lock `[:workspace-global :persistence-lock]`, `[:permissions :can-edit]` false, version preview `:preview-id`, lost render context `[:render-state :lost]`) always throws, whatever that flag. The `[:workspace-global :read-only?]` flag alone does not block: the Inspect tab sets it and plugins may still write. `r/check-permission` with `content:write`/`library:write` calls `r/check-writable` first, which covers most writers; writers without a permission check (most top-level context methods in `api.cljs`: `create*`, `group`, `align*`, …; z-order methods; text range `applyTypography`; ruler guide `remove`) call `r/check-writable` themselves, and shape setters get it through `u/check-shape-writable`. A new writer without a write-permission check must call it too. An async writer calls it again after each server await, before the next server write or commit (`rx/tap` on the response stream; `dwm/create-svg-shape-with-images` takes it as `:check-writable`), so a lock that lands mid-way rejects the promise with the read-only error. `r/has-permission?` is the check without it, used by the undo blocks so a block opened before a lock still closes. Comments (`comment:write`) stay writable: they do not go through file saves. - `penpotMgmt` (`app.plugins.management`) checks permissions with `r/has-permission?` (`granted?`), never `r/check-permission`: its calls act on files through the backend, often from the dashboard with no file open, so the workspace read-only check (`check-writable`) must not block them. - `naturalChildOrdering` and `throwValidationErrors` are stored per plugin under `[:plugins :flags plugin-id ...]`; changing default behavior affects automation and MCP diagnostics. - Plugin data is stored under keyword namespaces: private data uses `(keyword "plugin" plugin-id)`, shared data uses `(keyword "shared" namespace)`. diff --git a/frontend/src/app/plugins/register.cljs b/frontend/src/app/plugins/register.cljs index 072fb386a4..283b11a575 100644 --- a/frontend/src/app/plugins/register.cljs +++ b/frontend/src/app/plugins/register.cljs @@ -279,17 +279,34 @@ :else (let [{:keys [permissions]} (dm/get-in @registry [:data plugin-id])] (contains? permissions permission)))) +(defn- read-only-reason + "Why plugin writes must fail in `state`, or nil when they can go ahead. + The read-only flag alone is not a reason: the Inspect tab sets it too, + and it only hides the editing tools." + [state] + (let [wglobal (:workspace-global state)] + (cond + (some? (:persistence-lock wglobal)) + "because changes cannot be saved right now" + + (not (dm/get-in state [:permissions :can-edit])) + "because you cannot edit this file" + + (some? (:preview-id wglobal)) + "while a version is being previewed" + + (dm/get-in state [:render-state :lost]) + "because the canvas stopped rendering"))) + (defn check-writable - "Throws while the workspace is read-only: saves are failing, the user - cannot edit or a version is being previewed. A write made then would be - lost, so it fails whatever the plugin's validation flag." + "Throws while saves are failing, the user cannot edit, a version is + being previewed or the render context is lost. A write made then would + be lost or edit the wrong data, so it fails whatever the plugin's + validation flag." [] - (let [wglobal (:workspace-global @st/state)] - (when (true? (:read-only? wglobal)) - (throw (ex-info (if (some? (:persistence-lock wglobal)) - "[PENPOT PLUGIN] The workspace is read-only because changes cannot be saved right now" - "[PENPOT PLUGIN] The workspace is read-only") - {:type ::read-only}))))) + (when-let [reason (read-only-reason @st/state)] + (throw (ex-info (str "[PENPOT PLUGIN] The workspace is read-only " reason) + {:type ::read-only})))) (def ^:private write-permissions #{"content:write" "library:write"}) diff --git a/frontend/test/frontend_tests/plugins/read_only_test.cljs b/frontend/test/frontend_tests/plugins/read_only_test.cljs index 46dfa8db92..81532173fd 100644 --- a/frontend/test/frontend_tests/plugins/read_only_test.cljs +++ b/frontend/test/frontend_tests/plugins/read_only_test.cljs @@ -25,9 +25,9 @@ ;; Its validation flag is off, so only errors that always throw reach it (def ^:private plugin-id "00000000-0000-0000-0000-000000000000") -(defn- set-read-only - [store read-only?] - (swap! store assoc-in [:workspace-global :read-only?] read-only?)) +(defn- set-can-edit + [store can-edit?] + (swap! store assoc-in [:permissions :can-edit] can-edit?)) (defn- shape-count [store] @@ -46,7 +46,7 @@ (with-context (fn [store ^js context] (let [^js board (.createBoard context)] - (set-read-only store true) + (set-can-edit store false) (t/testing "a shape setter" (t/is (thrown-with-msg? js/Error #"read-only" (set! (.-name board) "Renamed"))) @@ -67,11 +67,31 @@ (with-context (fn [store ^js context] (let [^js board (.createBoard context)] - (set-read-only store true) - (set-read-only store false) + (set-can-edit store false) + (set-can-edit store true) (set! (.-name board) "Renamed") (t/is (= "Renamed" (.-name board))))))) +(t/deftest the-inspect-tab-does-not-block-writes + (with-context + (fn [store ^js context] + (let [^js board (.createBoard context)] + (swap! store update :workspace-global assoc + :read-only? true + :options-mode :inspect) + (set! (.-name board) "Renamed") + (t/is (= "Renamed" (.-name board))))))) + +(t/deftest each-read-only-source-says-why + (doseq [[path value reason] [[[:permissions :can-edit] false #"cannot edit this file"] + [[:workspace-global :preview-id] (random-uuid) #"version is being previewed"] + [[:render-state :lost] true #"canvas stopped rendering"]]] + (with-context + (fn [store ^js context] + (swap! store assoc-in path value) + (t/testing (str path) + (t/is (thrown-with-msg? js/Error reason (.createBoard context)))))))) + (t/deftest a-save-failure-lock-says-why-the-workspace-is-read-only (with-context (fn [store ^js context] @@ -84,7 +104,7 @@ (with-context (fn [store ^js context] (let [^js board (.createBoard context)] - (set-read-only store true) + (set-can-edit store false) (t/is (string? (.-name board))) (let [block (.undoBlockBegin (.-history context))] (t/is (some? block)) @@ -93,7 +113,7 @@ (t/deftest read-only-keeps-permission-checks-of-reads (with-context (fn [store _] - (set-read-only store true) + (set-can-edit store false) (t/is (true? (r/check-permission plugin-id "content:read"))) (t/is (true? (r/has-permission? plugin-id "content:write"))) (t/is (thrown? js/Error (r/check-permission plugin-id "library:write")))))) diff --git a/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs b/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs index 3d05d8abc1..cdd9d44017 100644 --- a/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs +++ b/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs @@ -102,7 +102,8 @@ emitted (atom nil) context (api/create-context plugin-id)] (set! st/state (atom {:current-file-id file-id - :current-page-id page-id})) + :current-page-id page-id + :permissions {:can-edit true}})) (with-redefs [dw/convert-selected-to-path (mock/stub (fn [ids] (reset! emitted ids) diff --git a/mcp/packages/server/data/initial_instructions.md b/mcp/packages/server/data/initial_instructions.md index 0ee464dbcd..e759903a7f 100644 --- a/mcp/packages/server/data/initial_instructions.md +++ b/mcp/packages/server/data/initial_instructions.md @@ -36,7 +36,9 @@ When you get this warning: * Redo the missing changes, then check again; if the same changes are rejected again, stop and tell the user. While saves are failing, the workspace becomes read-only: every write through the API throws an error saying that the -workspace is read-only. Do not retry in a loop; wait and try again later, or tell the user. +workspace is read-only because changes cannot be saved. Do not retry in a loop; wait and try again later, or tell the user. +Writes also throw when the user cannot edit the file, a version is being previewed or the canvas stopped rendering; +the error says which. These do not pass by waiting: tell the user. # The Structure of Penpot Designs diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 9bf4fc44f9..deef90a3e6 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -19,7 +19,7 @@ - **plugin-types, plugin-runtime:** Dashboard property setters now update values after backend success and log failures without interrupting the workspace. File moves reject projects in other teams, backend rejections include their type and code, and `project.fileCount` stays at its listing snapshot. Duplicated projects include their file count and pinned state. - **plugin-runtime:** Cancel plugin loads before they execute after unloading or logout, and allow reopening plugins that close during startup. - **plugins-runtime**: `library.connectLibrary()` now resolves once the library's assets are loaded; it used to resolve with an empty library the first time. `library.connected` and `library.availableLibraries()` ignore files opened earlier in the same tab, which used to show up as connected and be left out of the available libraries. -- **plugins-runtime:** Writes now fail with an error while the workspace is read-only (saves failing, a user who cannot edit, a version preview), whatever the `throwValidationErrors` flag. They used to change the workspace with edits that would never be saved. Undo blocks still open and close. +- **plugins-runtime:** Writes now fail with an error while the workspace cannot take them (saves failing, a user who cannot edit, a version preview, a canvas that stopped rendering), whatever the `throwValidationErrors` flag. They used to change the workspace with edits that would never be saved. The error says why. The Inspect tab does not block writes, and undo blocks still open and close. - **plugins-runtime:** Writes that wait for the server (`createShapeFromSvgWithImages`, `uploadMediaData`, `uploadMediaUrl`, `connectLibrary`) check again when the server answers. If the workspace turned read-only meanwhile (a save failed), the promise rejects with the read-only error: no shape is added and no library is linked. An image the server already stored stays in the file's media, unused. - **plugins-runtime:** `LibraryComponent.remove()` now rejects linked-library and missing component handles, and does nothing for an already deleted component. - **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.