From 5a0cee44b1b4860ecca4671732876782308f0fd9 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 27 Jul 2026 10:56:27 +0200 Subject: [PATCH 01/15] :bug: Harden frontend .getData call sites against undefined receivers (#10718) * :bug: Fix nil getData crash dropping ZIP without manifest.json Add nil-guard in read-as-text to raise typed :invalid-entry error instead of calling (.getData nil writer) which produced a raw TypeError. Made read-zip-manifest public (was defn-) with explicit detection of missing manifest.json, raising typed :invalid-penpot-file validation error. The existing catch path surfaces this hint as a friendly user error instead of the raw TypeError text. Add regression tests for both paths. 374 users were affected, 704 occurrences across 2.17.0-RC2/RC3/RC4. Fixes #10709. AI-assisted-by: minimax-m3 * :bug: Harden dnd/get-data against missing dataTransfer When the sortable hook or any caller passes a synthetic event without a dataTransfer property (e.g. a dragend fired after a drop that has already cleared the transfer), the previous implementation called .getData directly on the nil/undefined result and threw "Cannot read properties of undefined (reading 'getData')". Wrap the body in when-let so get-data returns nil cleanly when dataTransfer is missing. All three current callers (hooks.cljs:164, viewport/actions.cljs:531 and :562) already treat the return value as optional via when-let / when, so no caller breaks. Add a regression test covering both the missing-dataTransfer case and a real dataTransfer roundtrip. AI-assisted-by: minimax-m3 * :bug: Harden paste handler against missing clipboardData in forms When a paste event arrives without a clipboardData property (e.g. a programmatically dispatched ClipboardEvent in some browsers, or edge cases like dragging a file with no text content), the previous implementation called .getData directly on the nil/undefined clipboardData and threw "Cannot read properties of undefined (reading 'getData')". Wrap the body in when-let so the paste logic is skipped entirely when clipboardData is missing. The existing (string? paste-data) guard in the inner when already tolerates nil; no other caller behavior changes. AI-assisted-by: minimax-m3 * :bug: Harden paste handler against missing clipboardData in components/forms Same defensive pattern as the main/ui/forms.cljs paste handler: wrap the body in when-let so the .getData call is skipped when the clipboardData property is missing on the paste event. Prevents the raw "Cannot read properties of undefined (reading 'getData')" TypeError for programmatic / edge-case paste events. AI-assisted-by: minimax-m3 * :bug: Harden v3 text editor paste and styles-fn against undefined receivers Two related fixes for the "Cannot read properties of undefined (reading 'getData')" family of bugs in the workspace text editor: - v3_editor.cljs: wrap the paste body in when-let on clipboardData so .getData("text/plain") is never called on a nil receiver. The existing (when (and text (seq text))) guard already tolerates nil text; only the outer .getData call was unprotected. - editor.cljs: add (and content ...) to the if branch in styles-fn so .getText and .getData are never called on a nil content. The else branch (legacy.txt/styles-to-attrs) is already the correct fallback for missing content. Add a regression test that mirrors the fixed patterns and verifies they no longer throw on synthetic events with no clipboardData or nil content. AI-assisted-by: minimax-m3 * :bug: Harden get-editor-block-data and get-editor-block-type against nil block getCurrentBlock from Draft.js can return undefined for an empty selection (e.g. before any block is created). The previous implementations called .getData / .getType directly on the result and threw "Cannot read properties of undefined (reading 'getData')" / "...reading 'getType')". Wrap both functions in (when (some? block) ...) so they return nil cleanly. Callers in editor.cljs and text_editor.cljs already handle nil results (render-block short-circuits via the case on type; the text-data caller in text_editor.cljs lets nil flow up), so no upstream change is required. Add a regression test covering both functions with nil and js/undefined input. AI-assisted-by: minimax-m3 * :bug: Harden draft-js block-data helpers against nil block Three related fixes in the vendored draft-js package: - mergeBlockData: early-return undefined when block is falsy. Without this, the first line (block.getData()) throws for callers that pass a nil block. - splitBlockPreservingData: guard the blockMap.get(...) lookup. If the start key is stale (e.g. after a Modifier.splitBlock that doesn't actually produce the expected key), .get() returns undefined and the subsequent .getData() throws. Fall back to an empty Immutable Map for the block data. - updateBlockData: short-circuit (return state unchanged) when mergeBlockData returns undefined. Without this, the chain newBlock.getData() would throw on the same nil-block case that mergeBlockData now guards. These match the defensive nil-handling pattern used elsewhere in the frontend (.getData callers) and protect against stale selection keys in the Draft.js content state. AI-assisted-by: minimax-m3 --- frontend/packages/draft-js/index.js | 10 ++- .../src/app/main/ui/components/forms.cljs | 39 ++++++------ frontend/src/app/main/ui/forms.cljs | 39 ++++++------ .../main/ui/workspace/shapes/text/editor.cljs | 2 +- .../ui/workspace/shapes/text/v3_editor.cljs | 16 ++--- frontend/src/app/util/dom/dnd.cljs | 14 ++--- frontend/src/app/util/text_editor.cljs | 8 ++- frontend/src/app/util/zip.cljs | 4 ++ frontend/src/app/worker/import.cljs | 10 ++- frontend/test/frontend_tests/runner.cljs | 8 +++ .../text_editor_paste_guard_test.cljs | 62 +++++++++++++++++++ .../frontend_tests/util/dom/dnd_test.cljs | 23 +++++++ .../frontend_tests/util_text_editor_test.cljs | 18 ++++++ .../test/frontend_tests/util_zip_test.cljs | 44 +++++++++++++ 14 files changed, 235 insertions(+), 62 deletions(-) create mode 100644 frontend/test/frontend_tests/text_editor_paste_guard_test.cljs create mode 100644 frontend/test/frontend_tests/util/dom/dnd_test.cljs create mode 100644 frontend/test/frontend_tests/util_text_editor_test.cljs create mode 100644 frontend/test/frontend_tests/util_zip_test.cljs diff --git a/frontend/packages/draft-js/index.js b/frontend/packages/draft-js/index.js index 400636c5dd..f02109f971 100644 --- a/frontend/packages/draft-js/index.js +++ b/frontend/packages/draft-js/index.js @@ -29,6 +29,7 @@ function isDefined(v) { } function mergeBlockData(block, newData) { + if (!block) return undefined; let data = block.getData(); for (let key of Object.keys(newData)) { @@ -176,10 +177,12 @@ export function splitBlockPreservingData(state) { content = Modifier.splitBlock(content, selection); - const blockData = content.blockMap.get(content.selectionBefore.getStartKey()).getData(); + const startKey = content.selectionBefore.getStartKey(); + const block = content.blockMap.get(startKey); + const blockData = (block && block.getData()) || new Map(); const blockKey = content.selectionAfter.getStartKey(); - const blockMap = content.blockMap.update(blockKey, (block) => { - return block.set("data", blockData); + const blockMap = content.blockMap.update(blockKey, (b) => { + return b.set("data", blockData); }); content = content.set("blockMap", blockMap); @@ -325,6 +328,7 @@ export function updateBlockData(state, blockKey, data) { const content = state.getCurrentContent(); const block = content.getBlockForKey(blockKey); const newBlock = mergeBlockData(block, data); + if (!newBlock) return state; const blockData = newBlock.getData(); diff --git a/frontend/src/app/main/ui/components/forms.cljs b/frontend/src/app/main/ui/components/forms.cljs index 89a1bcd42d..88585a753e 100644 --- a/frontend/src/app/main/ui/components/forms.cljs +++ b/frontend/src/app/main/ui/components/forms.cljs @@ -560,28 +560,29 @@ on-paste (mf/use-fn (fn [event] - (let [paste-data (-> event .-clipboardData (.getData "text"))] - (when (and (string? paste-data) - (re-find #"[,\s]" paste-data)) - (dom/prevent-default event) - (dom/stop-propagation event) + (when-let [clipboard-data (.-clipboardData event)] + (let [paste-data (.getData clipboard-data "text")] + (when (and (string? paste-data) + (re-find #"[,\s]" paste-data)) + (dom/prevent-default event) + (dom/stop-propagation event) - ;; Mark as touched - (swap! form assoc-in [:touched input-name] true) + ;; Mark as touched + (swap! form assoc-in [:touched input-name] true) - ;; Split pasted text by commas and/or whitespace, add each valid part - (let [parts (->> (str/split paste-data #",|\s+") - (map str/trim) - (remove str/empty?))] - (doseq [part parts] - (when (valid-item-fn part) - (swap! items conj-dedup {:text part - :valid true - :caution (caution-item-fn part)}))) + ;; Split pasted text by commas and/or whitespace, add each valid part + (let [parts (->> (str/split paste-data #",|\s+") + (map str/trim) + (remove str/empty?))] + (doseq [part parts] + (when (valid-item-fn part) + (swap! items conj-dedup {:text part + :valid true + :caution (caution-item-fn part)}))) - ;; Reset input value and mark as untouched after successful paste - (reset! value "") - (swap! form assoc-in [:touched input-name] false)))))) + ;; Reset input value and mark as untouched after successful paste + (reset! value "") + (swap! form assoc-in [:touched input-name] false))))))) on-blur (mf/use-fn diff --git a/frontend/src/app/main/ui/forms.cljs b/frontend/src/app/main/ui/forms.cljs index 82b0335008..9115761290 100644 --- a/frontend/src/app/main/ui/forms.cljs +++ b/frontend/src/app/main/ui/forms.cljs @@ -121,28 +121,29 @@ on-paste (mf/use-fn (fn [event] - (let [paste-data (-> event .-clipboardData (.getData "text"))] - (when (and (string? paste-data) - (re-find #"[,\s]" paste-data)) - (dom/prevent-default event) - (dom/stop-propagation event) + (when-let [clipboard-data (.-clipboardData event)] + (let [paste-data (.getData clipboard-data "text")] + (when (and (string? paste-data) + (re-find #"[,\s]" paste-data)) + (dom/prevent-default event) + (dom/stop-propagation event) - ;; Mark as touched - (swap! form assoc-in [:touched name] true) + ;; Mark as touched + (swap! form assoc-in [:touched name] true) - ;; Split pasted text by commas and/or whitespace, add each valid part - (let [parts (->> (str/split paste-data #",|\s+") - (map str/trim) - (remove str/empty?))] - (doseq [part parts] - (when (valid-item-fn part) - (swap! items conj-dedup {:text part - :valid true - :caution (caution-item-fn part)}))) + ;; Split pasted text by commas and/or whitespace, add each valid part + (let [parts (->> (str/split paste-data #",|\s+") + (map str/trim) + (remove str/empty?))] + (doseq [part parts] + (when (valid-item-fn part) + (swap! items conj-dedup {:text part + :valid true + :caution (caution-item-fn part)}))) - ;; Reset input value and mark as untouched after successful paste - (reset! value "") - (swap! form assoc-in [:touched name] false)))))) + ;; Reset input value and mark as untouched after successful paste + (reset! value "") + (swap! form assoc-in [:touched name] false))))))) on-blur (mf/use-fn diff --git a/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs b/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs index 766acb2430..060ee0bada 100644 --- a/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs +++ b/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs @@ -61,7 +61,7 @@ nil))) (defn- styles-fn [shape styles content] - (let [data (if (= (.getText ^js content) "") + (let [data (if (and content (= (.getText ^js content) "")) (-> ^js (.getData content) (.toJS) (js->clj :keywordize-keys true)) diff --git a/frontend/src/app/main/ui/workspace/shapes/text/v3_editor.cljs b/frontend/src/app/main/ui/workspace/shapes/text/v3_editor.cljs index 749c5af69f..8db3275dad 100644 --- a/frontend/src/app/main/ui/workspace/shapes/text/v3_editor.cljs +++ b/frontend/src/app/main/ui/workspace/shapes/text/v3_editor.cljs @@ -105,14 +105,14 @@ (mf/use-fn (fn [^js event] (dom/prevent-default event) - (let [clipboard-data (.-clipboardData event) - text (.getData clipboard-data "text/plain")] - (when (and text (seq text)) - (text-editor/text-editor-insert-text text) - (sync-wasm-text-editor-content!) - (wasm.api/request-render "text-paste")) - (when-let [node (mf/ref-val contenteditable-ref)] - (set! (.-textContent node) ""))))) + (when-let [clipboard-data (.-clipboardData event)] + (let [text (.getData clipboard-data "text/plain")] + (when (and text (seq text)) + (text-editor/text-editor-insert-text text) + (sync-wasm-text-editor-content!) + (wasm.api/request-render "text-paste")))) + (when-let [node (mf/ref-val contenteditable-ref)] + (set! (.-textContent node) "")))) on-copy (mf/use-fn diff --git a/frontend/src/app/util/dom/dnd.cljs b/frontend/src/app/util/dom/dnd.cljs index 2a1732d6c2..bb647db785 100644 --- a/frontend/src/app/util/dom/dnd.cljs +++ b/frontend/src/app/util/dom/dnd.cljs @@ -115,13 +115,13 @@ ([e] (get-data e "penpot/data")) ([e data-type] - (let [dt (.-dataTransfer e) - data (.getData dt data-type)] - (cond-> data - (and (some? data) (not= data "") - (or (str/starts-with? data-type "penpot") - (= data-type "application/json"))) - (t/decode-str))))) + (when-let [dt (.-dataTransfer e)] + (let [data (.getData dt data-type)] + (cond-> data + (and (some? data) (not= data "") + (or (str/starts-with? data-type "penpot") + (= data-type "application/json"))) + (t/decode-str)))))) (defn get-files [e] diff --git a/frontend/src/app/util/text_editor.cljs b/frontend/src/app/util/text_editor.cljs index f46ed9aa35..4e91ee80ef 100644 --- a/frontend/src/app/util/text_editor.cljs +++ b/frontend/src/app/util/text_editor.cljs @@ -60,12 +60,14 @@ (defn get-editor-block-data [block] - (-> (.getData ^js block) - (immutable-map->map))) + (when (some? block) + (-> (.getData ^js block) + (immutable-map->map)))) (defn get-editor-block-type [block] - (.getType ^js block)) + (when (some? block) + (.getType ^js block))) (defn get-editor-current-block-data [state] diff --git a/frontend/src/app/util/zip.cljs b/frontend/src/app/util/zip.cljs index 72571f2e79..2288cbdaae 100644 --- a/frontend/src/app/util/zip.cljs +++ b/frontend/src/app/util/zip.cljs @@ -82,6 +82,10 @@ (defn read-as-text [entry] + (when (nil? entry) + (ex/raise :type :assertion + :code :invalid-entry + :hint "cannot read zip entry: entry is nil")) (let [writer (new zip/TextWriter)] (.getData entry writer))) diff --git a/frontend/src/app/worker/import.cljs b/frontend/src/app/worker/import.cljs index 91ce6b92c8..ba6a49ce1e 100644 --- a/frontend/src/app/worker/import.cljs +++ b/frontend/src/app/worker/import.cljs @@ -7,6 +7,7 @@ (ns app.worker.import (:refer-clojure :exclude [resolve]) (:require + [app.common.exceptions :as ex] [app.common.json :as json] [app.common.logging :as log] [app.common.schema :as sm] @@ -44,10 +45,15 @@ (def conjv (fnil conj [])) -(defn- read-zip-manifest +(defn read-zip-manifest [zip-reader] (->> (rx/from (uz/get-entry zip-reader "manifest.json")) - (rx/mapcat uz/read-as-text) + (rx/mapcat (fn [entry] + (if (nil? entry) + (rx/throw (ex/error :type :validation + :code :invalid-penpot-file + :hint "Not a valid Penpot file: manifest.json is missing")) + (uz/read-as-text entry)))) (rx/map json/decode))) (defn slurp-uri diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 128ce0c216..cda245671d 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -47,6 +47,7 @@ [frontend-tests.plugins.value-objects-test] [frontend-tests.render-wasm.process-objects-test] [frontend-tests.svg-fills-test] + [frontend-tests.text-editor-paste-guard-test] [frontend-tests.tokens.import-export-test] [frontend-tests.tokens.logic.token-actions-test] [frontend-tests.tokens.logic.token-data-test] @@ -60,7 +61,10 @@ [frontend-tests.util-object-test] [frontend-tests.util-range-tree-test] [frontend-tests.util-simple-math-test] + [frontend-tests.util-text-editor-test] [frontend-tests.util-webapi-test] + [frontend-tests.util-zip-test] + [frontend-tests.util.dom.dnd-test] [frontend-tests.worker-snap-test] [goog.object :as gobj])) @@ -132,10 +136,14 @@ 'frontend-tests.ui.ds-controls-numeric-input-test 'frontend-tests.ui.measures-menu-props-test 'frontend-tests.render-wasm.process-objects-test + 'frontend-tests.text-editor-paste-guard-test 'frontend-tests.util-object-test 'frontend-tests.util-range-tree-test 'frontend-tests.util-simple-math-test + 'frontend-tests.util-text-editor-test 'frontend-tests.util-webapi-test + 'frontend-tests.util.dom.dnd-test + 'frontend-tests.util-zip-test 'frontend-tests.worker-snap-test]) (assert (every? find-ns-obj test-namespaces) diff --git a/frontend/test/frontend_tests/text_editor_paste_guard_test.cljs b/frontend/test/frontend_tests/text_editor_paste_guard_test.cljs new file mode 100644 index 0000000000..3a7829f450 --- /dev/null +++ b/frontend/test/frontend_tests/text_editor_paste_guard_test.cljs @@ -0,0 +1,62 @@ +;; 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 + +(ns frontend-tests.text-editor-paste-guard-test + "Regression tests for the Cannot read properties of undefined + (reading getData) family of bugs. Each test verifies that the inner + getData call is now guarded by an outer when-let on clipboardData + so that a synthetic event with no clipboardData no longer throws." + (:require + [cljs.test :as t :include-macros true])) + +(defn- guarded-get-data-text + "Mirrors the body of the fixed paste handlers in main/ui/forms.cljs and + main/ui/components/forms.cljs." + [event] + (when-let [clipboard-data (.-clipboardData event)] + (.getData clipboard-data "text"))) + +(defn- guarded-get-data-text-plain + "Mirrors the body of the fixed paste handler in + main/ui/workspace/shapes/text/v3_editor.cljs." + [event] + (when-let [clipboard-data (.-clipboardData event)] + (.getData clipboard-data "text/plain"))) + +(t/deftest guarded-paste-handlers-do-not-throw-on-missing-clipboardData + (t/testing "event without clipboardData returns nil (no throw)" + (t/is (nil? (guarded-get-data-text #js {}))) + (t/is (nil? (guarded-get-data-text-plain #js {})))) + (t/testing "event with explicit nil clipboardData returns nil" + (t/is (nil? (guarded-get-data-text #js {:clipboardData nil}))) + (t/is (nil? (guarded-get-data-text-plain #js {:clipboardData nil})))) + (t/testing "event with valid clipboardData returns the text" + (let [text-cb (fn [t] (if (= t "text") "hello" nil)) + text-plain-cb (fn [t] (if (= t "text/plain") "hello" nil)) + cb #js {:getData (fn [t] (if (= t "text") "hello" nil))} + cbp #js {:getData (fn [t] (if (= t "text/plain") "hello" nil))}] + (t/is (= "hello" (guarded-get-data-text #js {:clipboardData cb}))) + (t/is (= "hello" (guarded-get-data-text-plain #js {:clipboardData cbp})))))) + +;; Mirrors the fixed styles-fn body in main/ui/workspace/shapes/text/editor.cljs. +;; The fix adds an (and content ...) guard so getText and getData are never +;; called on a nil content object. +(defn- guarded-styles-fn-branch + "Returns the data that styles-fn would use for a given content. When content + is nil, the function falls back to the styles-only branch and returns :fallback + (the real function calls legacy.txt/styles-to-attrs which we don't exercise + here — we only verify the guard itself prevents the getText/getData throws)." + [content] + (if (and content (= (.getText ^js content) "")) + (-> ^js (.getData content) + (.toJS) + (js->clj :keywordize-keys true)) + :fallback)) + +(t/deftest guarded-styles-fn-branch-does-not-throw-on-nil-content + (t/testing "nil content falls back to the styles branch (no throw)" + (t/is (= :fallback (guarded-styles-fn-branch nil))) + (t/is (= :fallback (guarded-styles-fn-branch js/undefined))))) diff --git a/frontend/test/frontend_tests/util/dom/dnd_test.cljs b/frontend/test/frontend_tests/util/dom/dnd_test.cljs new file mode 100644 index 0000000000..60c7ad725d --- /dev/null +++ b/frontend/test/frontend_tests/util/dom/dnd_test.cljs @@ -0,0 +1,23 @@ +;; 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 + +(ns frontend-tests.util.dom.dnd-test + (:require + [app.util.dom.dnd :as dnd] + [cljs.test :as t :include-macros true])) + +(t/deftest get-data-returns-nil-when-event-has-no-dataTransfer + (t/testing "event without dataTransfer" + (t/is (nil? (dnd/get-data #js {})))) + (t/testing "event with explicit nil dataTransfer" + (t/is (nil? (dnd/get-data #js {:dataTransfer nil})))) + (t/testing "explicit data-type also returns nil for missing dataTransfer" + (t/is (nil? (dnd/get-data #js {} "penpot/data"))))) + +(t/deftest get-data-reads-from-dataTransfer + (t/testing "dataTransfer with matching key returns the value (non-decoded type)" + (let [dt #js {:getData (fn [_type] "hello")}] + (t/is (= "hello" (dnd/get-data #js {:dataTransfer dt} "text/plain")))))) diff --git a/frontend/test/frontend_tests/util_text_editor_test.cljs b/frontend/test/frontend_tests/util_text_editor_test.cljs new file mode 100644 index 0000000000..0823a77d10 --- /dev/null +++ b/frontend/test/frontend_tests/util_text_editor_test.cljs @@ -0,0 +1,18 @@ +;; 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 + +(ns frontend-tests.util-text-editor-test + (:require + [app.util.text-editor :as te] + [cljs.test :as t :include-macros true])) + +(t/deftest get-editor-block-data-returns-nil-for-nil-block + (t/is (nil? (te/get-editor-block-data nil))) + (t/is (nil? (te/get-editor-block-data js/undefined)))) + +(t/deftest get-editor-block-type-returns-nil-for-nil-block + (t/is (nil? (te/get-editor-block-type nil))) + (t/is (nil? (te/get-editor-block-type js/undefined)))) diff --git a/frontend/test/frontend_tests/util_zip_test.cljs b/frontend/test/frontend_tests/util_zip_test.cljs new file mode 100644 index 0000000000..7cf2d6f609 --- /dev/null +++ b/frontend/test/frontend_tests/util_zip_test.cljs @@ -0,0 +1,44 @@ +;; 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 + +(ns frontend-tests.util-zip-test + (:require + [app.util.zip :as-alias uz] + [app.worker.import :as worker.import] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.mock :as mock] + [promesa.core :as p])) + +(t/deftest read-as-text-nil-entry-raises-typed-error + (t/testing "read-as-text guards against nil entry" + (t/is (thrown-with-msg? js/Error #"nil" + (app.util.zip/read-as-text nil))))) + +(t/deftest read-zip-manifest-missing-throws-validation-error + (t/async + done + (t/testing "read-zip-manifest rejects ZIPs without manifest.json" + (mock/with-mocks + {app.util.zip/get-entry (mock/stub (fn [_ _] (p/resolved nil)))} + (fn [done'] + (->> (worker.import/read-zip-manifest #js {}) + (rx/subs! + (fn [_] + (t/is false "expected validation error to be thrown") + (done')) + (fn [err] + (let [data (ex-data err)] + (t/is (= :invalid-penpot-file (:code data)) + "missing manifest.json raises typed :invalid-penpot-file error") + (t/is (string? (:hint data)) + "missing manifest.json error carries a :hint") + (t/is (re-find #"manifest\.json" (:hint data)) + "missing manifest.json :hint mentions manifest.json") + (t/is (nil? (re-find #"getData" (:hint data))) + "missing manifest.json :hint does not leak the raw TypeError text") + (done')))))) + done)))) \ No newline at end of file From af120feb1f86bffeb1abcea2f9b0717e3c338e82 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 27 Jul 2026 12:11:12 +0200 Subject: [PATCH 02/15] :bug: Fix workspace crash and cleanup viewport_ref event/resize handling (#10721) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace `globals/document` and `globals/window` with `js/document` and `js/window` in workspace.cljs, removing the unused `app.util.globals` import. This avoids "can't access dead object" errors in Firefox when navigating between pages/files, matching the existing pattern used in viewport/hooks.cljs. Fix a leaked MOUSELEAVE listener in viewport_ref.cljs — the ref callback added a new listener on every mount but never unregistered the previous one. Now uses standard .addEventListener/.removeEventListener with a React ref to track the handler for proper cleanup. Fix ResizeObserver cleanup in viewport_ref.cljs — `init-observer` is now a private function that only creates an observer when a node is provided, and cleanup is handled via the ref callback on unmount. AI-assisted-by: mimo-v2.5-pro --- frontend/src/app/main/ui/workspace.cljs | 5 +- .../ui/workspace/viewport/viewport_ref.cljs | 95 +++++++++++-------- 2 files changed, 56 insertions(+), 44 deletions(-) diff --git a/frontend/src/app/main/ui/workspace.cljs b/frontend/src/app/main/ui/workspace.cljs index e7a3e7b119..124eee9b1b 100644 --- a/frontend/src/app/main/ui/workspace.cljs +++ b/frontend/src/app/main/ui/workspace.cljs @@ -44,7 +44,6 @@ [app.main.ui.workspace.webgl-unavailable-modal] [app.util.debug :as dbg] [app.util.dom :as dom] - [app.util.globals :as globals] [app.util.i18n :as i18n :refer [tr]] [goog.events :as events] [okulary.core :as l] @@ -177,7 +176,7 @@ (mf/with-effect [] (let [focus-out #(st/emit! (dw/workspace-focus-lost)) - key (events/listen globals/window "blur" focus-out)] + key (events/listen js/window "blur" focus-out)] (partial events/unlistenByKey key))) (mf/with-effect [file-id page-id] @@ -252,7 +251,7 @@ (let [handle-wasm-render (fn [_] (reset! first-frame-rendered? true)) - listener-key (events/listen globals/document "penpot:wasm:render" handle-wasm-render)] + listener-key (events/listen js/document "penpot:wasm:render" handle-wasm-render)] (fn [] (events/unlistenByKey listener-key)))) diff --git a/frontend/src/app/main/ui/workspace/viewport/viewport_ref.cljs b/frontend/src/app/main/ui/workspace/viewport/viewport_ref.cljs index 46e1356596..38349f8f24 100644 --- a/frontend/src/app/main/ui/workspace/viewport/viewport_ref.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/viewport_ref.cljs @@ -7,62 +7,75 @@ (ns app.main.ui.workspace.viewport.viewport-ref (:require [app.common.data :as d] - [app.common.data.macros :as dm] [app.common.geom.point :as gpt] + [app.main.refs :as refs] [app.main.store :as st] [app.util.dom :as dom] [app.util.mouse :as mse] - [goog.events :as events] - [rumext.v2 :as mf]) - (:import goog.events.EventType)) + [rumext.v2 :as mf])) (defonce viewport-ref (atom nil)) -(defonce current-observer (atom nil)) (defonce viewport-brect (atom nil)) -(defn init-observer - [node on-change-bounds] +(defn- init-observer + [node] + (let [on-change-bounds + (fn [_] + (let [brect (dom/get-bounding-rect node) + brect (gpt/point (d/parse-integer (:left brect)) + (d/parse-integer (:top brect)))] + (reset! viewport-brect brect))) - (let [observer (js/ResizeObserver. on-change-bounds)] - (when (some? @current-observer) - (.disconnect @current-observer)) + observer + (js/ResizeObserver. on-change-bounds)] - (reset! current-observer observer) - - (when (some? node) - (.observe observer node)))) - -(defn on-change-bounds - [_] - (when @viewport-ref - (let [brect (dom/get-bounding-rect @viewport-ref) - brect (gpt/point (d/parse-integer (:left brect)) - (d/parse-integer (:top brect)))] - (reset! viewport-brect brect)))) + (.observe observer node) + observer)) (defn create-viewport-ref [] - (let [ref (mf/use-ref nil)] - [ref - (mf/use-memo - #(fn [node] - (mf/set-ref-val! ref node) - (reset! viewport-ref node) - (when (some? node) - (events/listen node EventType.MOUSELEAVE (fn [] (st/emit! (mse/->BlurEvent))))) - (init-observer node on-change-bounds)))])) + (let [node-ref (mf/use-ref nil) + handler-ref (mf/use-ref nil) + observer-ref (mf/use-ref nil) + callback (mf/use-fn + (fn [node] + ;; Dispose all previous resources + (when-let [observer (mf/ref-val observer-ref)] + (.disconnect ^js observer) + (mf/set-ref-val! observer-ref nil)) + + + (when-let [handler (mf/ref-val handler-ref)] + (when-let [node (mf/ref-val node-ref)] + (.removeEventListener ^js node "mouseleave" handler) + (mf/set-ref-val! handler-ref nil))) + + ;; Reset the ref values to the current node (can be nil) + (mf/set-ref-val! node-ref node) + (reset! viewport-ref node) + + (when (some? node) + (let [handler (fn [] (st/emit! (mse/->BlurEvent))) + observer (init-observer node)] + (.addEventListener ^js node "mouseleave" handler) + + (mf/set-ref-val! handler-ref handler) + (mf/set-ref-val! observer-ref observer)))))] + [node-ref callback])) (defn point->viewport [pt] - (let [zoom (dm/get-in @st/state [:workspace-local :zoom] 1)] - (when (and (some? @viewport-ref) - (some? @viewport-brect)) - (let [vbox (.. ^js @viewport-ref -viewBox -baseVal) - brect @viewport-brect - box (gpt/point (.-x vbox) (.-y vbox)) - zoom (gpt/point zoom)] + (let [zoom (d/nilv @refs/selected-zoom 1) + viewport-node @viewport-ref + viewport-brect @viewport-brect] - (-> (gpt/subtract pt brect) + (when (and (some? viewport-brect) + (some? viewport-node)) + (let [vbox (.. ^js viewport-node -viewBox -baseVal) + box (gpt/point (.-x vbox) (.-y vbox)) + zoom (gpt/point zoom)] + + (-> (gpt/subtract pt viewport-brect) (gpt/divide zoom) (gpt/add box)))))) @@ -71,8 +84,8 @@ Unlike point->viewport, this does NOT convert to canvas coordinates - it just subtracts the viewport's bounding rect offset." [pt] - (when (some? @viewport-brect) - (gpt/subtract pt @viewport-brect))) + (when-let [brect @viewport-brect] + (gpt/subtract pt brect))) (defn inside-viewport? [target] From 63e0c536f0caed806e9087094dbb1281caaf44a5 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 27 Jul 2026 13:24:11 +0000 Subject: [PATCH 03/15] :sparkles: Align event names column in format-last-events output The third column (event name) in error report "last events" now starts at a consistent position regardless of the delta value, by right-padding the delta string to 10 characters. The first event always shows (+0ms). Adds tests for empty, single, multi-event, and column alignment cases. AI-assisted-by: deepseek-v4-flash --- frontend/src/app/main/store.cljs | 15 +++--- .../test/frontend_tests/data/store_test.cljs | 49 +++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 3 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 frontend/test/frontend_tests/data/store_test.cljs diff --git a/frontend/src/app/main/store.cljs b/frontend/src/app/main/store.cljs index 9ecfc39284..dc389033ae 100644 --- a/frontend/src/app/main/store.cljs +++ b/frontend/src/app/main/store.cljs @@ -107,8 +107,8 @@ (defn format-last-events "Render the `last-events` buffer as a multi-line string with the wall-clock time of each event and the delta (ms) since the previous - entry. The first entry has no delta. Useful for embedding in error - reports." + entry. The delta column is right-padded to 10 chars so the event + names align. Useful for embedding in error reports." ([] (format-last-events @last-events)) ([events] (let [lines @@ -117,13 +117,14 @@ out (transient [])] (if xs (let [{:keys [name t]} (first xs) - iso (ct/format-inst t :iso) - tail (if prev-t - (str " (+" (ct/diff-ms prev-t t) "ms)") - "")] + iso (ct/format-inst t :iso) + delta (if prev-t + (str "(+" (ct/diff-ms prev-t t) "ms)") + "(+0ms)") + delta-pad (str/pad delta {:length 10 :type :right})] (recur t (next xs) - (conj! out (str iso tail " " name)))) + (conj! out (str iso " " delta-pad " " name)))) (persistent! out)))] (str/join "\n" lines)))) diff --git a/frontend/test/frontend_tests/data/store_test.cljs b/frontend/test/frontend_tests/data/store_test.cljs new file mode 100644 index 0000000000..56d29fec85 --- /dev/null +++ b/frontend/test/frontend_tests/data/store_test.cljs @@ -0,0 +1,49 @@ +;; 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.data.store-test + "Unit tests for app.main.store. + Tests cover: + - format-last-events – empty, single, multi, column alignment" + (:require + [app.main.store :as st] + [cljs.test :as t :include-macros true] + [cuerdas.core :as str])) + +(t/deftest format-last-events-empty + (t/testing "empty events produce empty string" + (t/is (= "" (st/format-last-events []))))) + +(t/deftest format-last-events-single + (t/testing "a single event shows (+0ms) and its name" + (let [result (st/format-last-events [{:name ":test/event" :t (js/Date. 1000)}])] + (t/is (str/includes? result "(+0ms)")) + (t/is (str/includes? result ":test/event")) + (t/is (= 1 (count (str/split result "\n"))))))) + +(t/deftest format-last-events-multi + (t/testing "multiple events show correct deltas and event names" + (let [events [{:name ":event/a" :t (js/Date. 0)} + {:name ":event/b" :t (js/Date. 500)} + {:name ":event/c" :t (js/Date. 2500)}] + lines (str/split (st/format-last-events events) "\n")] + (t/is (= 3 (count lines))) + (t/is (some #(str/includes? % ":event/a") lines)) + (t/is (some #(str/includes? % ":event/b") lines)) + (t/is (some #(str/includes? % ":event/c") lines)) + (t/is (str/includes? (nth lines 0) "(+0ms)")) + (t/is (str/includes? (nth lines 1) "(+500ms)")) + (t/is (str/includes? (nth lines 2) "(+2000ms)"))))) + +(t/deftest format-last-events-alignment + (t/testing "event names start at the same column across all lines" + (let [events [{:name ":evt-a" :t (js/Date. 0)} + {:name ":evt-b" :t (js/Date. 500)}] + lines (str/split (st/format-last-events events) "\n") + col-a (.indexOf (nth lines 0) ":evt-a") + col-b (.indexOf (nth lines 1) ":evt-b")] + (t/is (pos? col-a)) + (t/is (= col-a col-b))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index cda245671d..450f6978bd 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -9,6 +9,7 @@ [frontend-tests.copy-as-svg-test] [frontend-tests.data.nitrate-test] [frontend-tests.data.repo-test] + [frontend-tests.data.store-test] [frontend-tests.data.uploads-test] [frontend-tests.data.viewer-test] [frontend-tests.data.workspace-colors-test] @@ -86,6 +87,7 @@ 'frontend-tests.copy-as-svg-test 'frontend-tests.data.nitrate-test 'frontend-tests.data.repo-test + 'frontend-tests.data.store-test 'frontend-tests.errors-test 'frontend-tests.main-errors-test 'frontend-tests.data.uploads-test From 6c2b61e1ad6912f556b0e297c13a3a8a262b182a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 28 Jul 2026 10:59:16 +0200 Subject: [PATCH 04/15] :bug: Fix several issues in RPC command handlers (#10670) * :bug: Fix several issues in RPC command handlers - Reject circular library references in link-file-to-library - Add explicit team permission check in search-files - Constrain search-term max length to 250 chars - Include :deleted-at in file ETag for COND caching - Move storage I/O outside DB transaction in create-file-thumbnail AI-assisted-by: deepseek-v4-pro * :paperclip: Check perms before circular link checks * :bug: Handle circular library reference error Catch :circular-library-reference error from backend when linking files to libraries. Show user-friendly toast notification instead of propagating unhandled error. Add English and Spanish translations. AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/files.clj | 13 +- .../src/app/rpc/commands/files_thumbnails.clj | 116 ++++++++---------- backend/src/app/rpc/commands/search.clj | 8 +- backend/test/backend_tests/rpc_file_test.clj | 72 +++++++++++ .../app/main/data/workspace/libraries.cljs | 7 +- frontend/translations/en.po | 3 + frontend/translations/es.po | 3 + 7 files changed, 154 insertions(+), 68 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index 3851ea577b..4daa2dd32a 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -156,11 +156,13 @@ (assoc mfile :permissions perms))) (defn get-file-etag - [{:keys [::rpc/profile-id]} {:keys [modified-at revn vern permissions]}] + [{:keys [::rpc/profile-id]} {:keys [modified-at revn vern deleted-at permissions]}] (str profile-id "/" revn "/" vern "/" (hash fmg/available-migrations) "/" (ct/format-inst modified-at :iso) "/" - (uri/map->query-string permissions))) + (uri/map->query-string permissions) + "/" + (some-> deleted-at (ct/format-inst :iso)))) (sv/defmethod ::get-file "Retrieve a file by its ID. Only authenticated users." @@ -1102,6 +1104,13 @@ (check-edition-permissions! conn profile-id file-id) (check-edition-permissions! conn profile-id library-id) + + (let [transitive-deps (bfc/get-libraries cfg [library-id])] + (when (contains? transitive-deps file-id) + (ex/raise :type :validation + :code :circular-library-reference + :hint "linking this library would create a circular dependency"))) + (link-file-to-library conn params) (bfc/get-libraries cfg [library-id])) diff --git a/backend/src/app/rpc/commands/files_thumbnails.clj b/backend/src/app/rpc/commands/files_thumbnails.clj index 130d9a86be..024bce17e7 100644 --- a/backend/src/app/rpc/commands/files_thumbnails.clj +++ b/backend/src/app/rpc/commands/files_thumbnails.clj @@ -374,61 +374,6 @@ ;; --- MUTATION COMMAND: create-file-thumbnail -(defn- create-file-thumbnail - [{:keys [::db/conn ::sto/storage] :as cfg} {:keys [file-id revn props media] :as params}] - (media/validate-media-type! media) - (media/validate-media-size! media) - - (let [file (bfc/get-file cfg file-id - :include-deleted? true - :load-data? false) - - props (db/tjson (or props {})) - path (:path media) - mtype (:mtype media) - hash (sto/calculate-hash path) - data (-> (sto/content path) - (sto/wrap-with-hash hash)) - tnow (ct/now) - - media (sto/put-object! storage - {::sto/content data - ::sto/deduplicate? true - ::sto/touched-at tnow - :content-type mtype - :bucket "file-thumbnail"}) - - thumb (db/get* conn :file-thumbnail - {:file-id file-id - :revn revn} - {::db/remove-deleted false - ::sql/for-update true})] - - (if (some? thumb) - (do - ;; We mark the old media id as touched if it does not match - (when (not= (:id media) (:media-id thumb)) - (sto/touch-object! storage (:media-id thumb))) - - (db/update! conn :file-thumbnail - {:media-id (:id media) - :deleted-at (:deleted-at file) - :updated-at tnow - :props props} - {:file-id file-id - :revn revn})) - - (db/insert! conn :file-thumbnail - {:file-id file-id - :revn revn - :created-at tnow - :updated-at tnow - :deleted-at (:deleted-at file) - :props props - :media-id (:id media)})) - - media)) - (def ^:private schema:create-file-thumbnail [:map {:title "create-file-thumbnail"} @@ -448,12 +393,57 @@ ::rtry/when rtry/conflict-exception? ::sm/params schema:create-file-thumbnail} - ;; FIXME: do not run the thumbnail upload inside a transaction - [cfg {:keys [::rpc/profile-id file-id] :as params}] - (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] - (files/check-edition-permissions! conn profile-id file-id) - (when-not (db/read-only? conn) - (let [media (create-file-thumbnail cfg params)] - {:uri (files/resolve-public-uri (:id media)) - :id (:id media)}))))) + (media/validate-media-type! (:media params)) + (media/validate-media-size! (:media params)) + + (db/run! cfg files/check-edition-permissions! profile-id file-id) + + (when-not (db/read-only? (::db/pool cfg)) + (let [storage (::sto/storage cfg) + file (bfc/get-file cfg file-id :include-deleted? true :load-data? false) + props (db/tjson (or (:props params) {})) + {:keys [path mtype]} (:media params) + hash (sto/calculate-hash path) + data (-> (sto/content path) + (sto/wrap-with-hash hash)) + tnow (ct/now) + + media (sto/put-object! storage + {::sto/content data + ::sto/deduplicate? true + ::sto/touched-at tnow + :content-type mtype + :bucket "file-thumbnail"}) + + revn (:revn params) + + result (db/tx-run! cfg + (fn [{:keys [::db/conn]}] + (let [thumb (db/get* conn :file-thumbnail + {:file-id file-id :revn revn} + {::db/remove-deleted false + ::sql/for-update true})] + (if (some? thumb) + (do + (when (not= (:id media) (:media-id thumb)) + (sto/touch-object! storage (:media-id thumb))) + (db/update! conn :file-thumbnail + {:media-id (:id media) + :deleted-at (:deleted-at file) + :updated-at tnow + :props props} + {:file-id file-id :revn revn})) + (db/insert! conn :file-thumbnail + {:file-id file-id + :revn revn + :created-at tnow + :updated-at tnow + :deleted-at (:deleted-at file) + :props props + :media-id (:id media)})) + media)))] + + (when result + {:uri (files/resolve-public-uri (:id result)) + :id (:id result)})))) diff --git a/backend/src/app/rpc/commands/search.clj b/backend/src/app/rpc/commands/search.clj index 796ddf4810..7b60e6db30 100644 --- a/backend/src/app/rpc/commands/search.clj +++ b/backend/src/app/rpc/commands/search.clj @@ -6,9 +6,11 @@ (ns app.rpc.commands.search (:require + [app.common.data.macros :as dm] [app.common.schema :as sm] [app.db :as db] [app.rpc :as-alias rpc] + [app.rpc.commands.teams :as teams] [app.rpc.doc :as-alias doc] [app.util.services :as sv])) @@ -66,11 +68,13 @@ (def ^:private schema:search-files [:map {:title "search-files"} [:team-id ::sm/uuid] - [:search-term {:optional true} :string]]) + [:search-term {:optional true} [:string {:max 250}]]]) (sv/defmethod ::search-files {::doc/added "1.17" ::doc/module :files ::sm/params schema:search-files} [{:keys [::db/pool]} {:keys [::rpc/profile-id team-id search-term]}] - (some->> search-term (search-files pool profile-id team-id))) + (dm/with-open [conn (db/open pool)] + (teams/check-read-permissions! conn profile-id team-id) + (some->> search-term (search-files conn profile-id team-id)))) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index 5460b4143f..1c07f35971 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -2319,3 +2319,75 @@ (t/is (not (nil? (:error out)))) (let [edata (-> out :error ex-data)] (t/is (= :not-found (:type edata)))))) + +;; --- Security Fix Tests --- + +(t/deftest link-file-to-library-circular-reference + (let [profile (th/create-profile* 1) + file1 (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared true}) + file2 (th/create-file* 2 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared true}) + file3 (th/create-file* 3 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false})] + (th/link-file-to-library* {:file-id (:id file3) :library-id (:id file2)}) + (th/link-file-to-library* {:file-id (:id file2) :library-id (:id file1)}) + (let [data {::th/type :link-file-to-library + ::rpc/profile-id (:id profile) + :file-id (:id file1) + :library-id (:id file3)} + out (th/command! data)] + (t/is (th/ex-info? (:error out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (let [edata (-> out :error ex-data)] + (t/is (= :circular-library-reference (:code edata))))))) + +(t/deftest get-file-etag-includes-deleted-at + (let [profile-id (uuid/random) + file1 {:modified-at (ct/now) + :revn 1 + :vern 0 + :deleted-at nil + :permissions {:can-edit true}} + file2 (assoc file1 :deleted-at (ct/now))] + (t/is (not= (files/get-file-etag {::rpc/profile-id profile-id} file1) + (files/get-file-etag {::rpc/profile-id profile-id} file2))))) + +(t/deftest search-files-with-permission + (let [profile (th/create-profile* 1) + _ (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + data {::th/type :search-files + ::rpc/profile-id (:id profile) + :team-id (:default-team-id profile) + :search-term "test"} + out (th/command! data)] + (t/is (nil? (:error out))) + (t/is (vector? (:result out))))) + +(t/deftest search-files-forbidden + (let [profile (th/create-profile* 1) + other (th/create-profile* 2) + data {::th/type :search-files + ::rpc/profile-id (:id other) + :team-id (:default-team-id profile) + :search-term "test"} + out (th/command! data)] + (t/is (th/ex-info? (:error out))) + (let [edata (-> out :error ex-data)] + (t/is (= :not-found (:type edata)))))) + +(t/deftest search-files-term-too-long + (let [profile (th/create-profile* 1) + data {::th/type :search-files + ::rpc/profile-id (:id profile) + :team-id (:default-team-id profile) + :search-term (apply str (repeat 300 "x"))} + out (th/command! data)] + (t/is (th/ex-info? (:error out))) + (let [edata (-> out :error ex-data)] + (t/is (= :validation (:type edata)))))) diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index 667f0b2b61..5c09149bad 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -1571,7 +1571,12 @@ (as-> libraries-to-load $ (remove loaded-libraries $) (conj $ library-id) - (map #(load-library-file file-id %) $)))))) + (map #(load-library-file file-id %) $)))) + (rx/catch (fn [cause] + (let [error (ex-data cause)] + (if (= (:code error) :circular-library-reference) + (rx/of (ntf/error (tr "errors.circular-library-reference"))) + (rx/throw cause))))))) (rx/of (ptk/reify ::attach-library-finished)) (when (pos? variants-count) (->> (rp/cmd! :get-library-usage {:file-id library-id}) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 2155b71895..1f0017d87f 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1490,6 +1490,9 @@ msgstr "The fonts %s could not be loaded" msgid "errors.cannot-upload" msgstr "Cannot upload the media file." +msgid "errors.circular-library-reference" +msgstr "Cannot add library: this would create a circular dependency" + #: src/app/main/ui/comments.cljs:737, src/app/main/ui/comments.cljs:767, src/app/main/ui/comments.cljs:864 msgid "errors.character-limit-exceeded" msgstr "Character limit exceeded" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index ef0ef18989..3c2655035b 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -1501,6 +1501,9 @@ msgstr "No se han podido cargar las fuentes %s" msgid "errors.cannot-upload" msgstr "No se puede cargar el archivo multimedia." +msgid "errors.circular-library-reference" +msgstr "No se puede añadir la biblioteca: crearía una dependencia circular" + #: src/app/main/ui/comments.cljs:737, src/app/main/ui/comments.cljs:767, src/app/main/ui/comments.cljs:864 msgid "errors.character-limit-exceeded" msgstr "Se ha superado el límite de caracteres" From 4b994d20aac78723770ae6538bd3372b7936add1 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 28 Jul 2026 11:06:00 +0200 Subject: [PATCH 05/15] :bug: Fix leaked focus timers in dashboard sidebar navigation (#10715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :bug: Fix leaked deferred DOM ops on dashboard navigation and template clone The React reconciliation "removeChild" error surfaced during rapid dashboard navigation because several effects scheduled deferred DOM operations (focus, CSS positioning) without returning a cleanup that cancelled them. When the component unmounted before the callback fired, it ran against stale DOM and desynchronized React fiber tree from the actual DOM. - context_menu_a11y.cljs: replace tm/schedule-on-idle (30s idle window) with tm/schedule (setTimeout 0) and return a rx/dispose! cleanup. - dropdown.cljs: capture the tm/schedule handle and dispose it in the effect cleanup. - tooltip.cljs: capture the ts/raf handle and cancel it on cleanup. AI-assisted-by: opencode-go/mimo-v2.5-pro * :bug: Fix leaked focus timers in dashboard sidebar navigation Six sidebar navigation handlers scheduled setTimeout callbacks to mutate tabindex/focus on React-managed title elements without cancelling prior pending callbacks. During rapid keyboard navigation (Projects→Fonts→Libs→Drafts) the stale callbacks fired against unmounted DOM, desyncing React fiber tree and triggering "removeChild" NotFoundError. - sidebar-project*: cancel prior timer in on-key-down - sidebar-search*: cancel prior timer in on-key-press - sidebar-content*: cancel prior timer in go-projects-with-key, go-fonts-with-key, go-drafts-with-key, go-libs-with-key Each handler now stores the timer handle in a component-level ref and disposes any pending handle before scheduling a new one. AI-assisted-by: opencode-go/mimo-v2.5-pro * :recycle: Refactor sidebar focus timer handling into helpers Extract the repeated dispose-before-schedule focus idiom into schedule-focus-by-id! (sidebar.cljs) and focus-and-untabbable! (app.util.dom). Replaces the six duplicated blocks and adds mf/use-effect unmount cleanup to dispose any pending timer in the three sidebar components, closing the remaining leak noted in the original fix. AI-assisted-by: opencode/hy3-free * :recycle: Extract use-focus-timer-ref hook for sidebar components Replace the duplicated mf/use-ref + mf/use-effect cleanup pairs in sidebar-project*, sidebar-search*, and sidebar-content* with a shared use-focus-timer-ref hook (app.main.ui.hooks). The hook creates the ref and disposes any pending timer on unmount via mf/with-effect, reading the ref with mf/ref-val instead of deref. mf/use-effect is now a body-level hook call rather than a let binding. AI-assisted-by: opencode/hy3-free * :paperclip: Add pr feedback fix --- .../main/ui/components/context_menu_a11y.cljs | 8 ++- .../src/app/main/ui/components/dropdown.cljs | 8 ++- .../src/app/main/ui/dashboard/sidebar.cljs | 64 +++++++------------ .../src/app/main/ui/ds/tooltip/tooltip.cljs | 37 +++++------ frontend/src/app/main/ui/hooks.cljs | 10 +++ frontend/src/app/util/dom.cljs | 7 ++ 6 files changed, 70 insertions(+), 64 deletions(-) diff --git a/frontend/src/app/main/ui/components/context_menu_a11y.cljs b/frontend/src/app/main/ui/components/context_menu_a11y.cljs index 75c97554ce..e2e824c19e 100644 --- a/frontend/src/app/main/ui/components/context_menu_a11y.cljs +++ b/frontend/src/app/main/ui/components/context_menu_a11y.cljs @@ -18,6 +18,7 @@ [app.util.i18n :as i18n :refer [tr]] [app.util.keyboard :as kbd] [app.util.timers :as tm] + [beicon.v2.core :as rx] [rumext.v2 :as mf])) (def ^:private xf:options @@ -229,8 +230,11 @@ (partial ug/unlisten "penpot:context-menu:open" on-event))) (mf/with-effect [ids] - (tm/schedule-on-idle - #(dom/focus! (dom/get-element (first ids))))) + (let [handle (tm/schedule + (fn [] + (some-> (dom/get-element (first ids)) + (dom/focus!))))] + #(rx/dispose! handle))) (when (some? levels) [:> dropdown-content* props diff --git a/frontend/src/app/main/ui/components/dropdown.cljs b/frontend/src/app/main/ui/components/dropdown.cljs index 894d9ef204..4a0a1b0590 100644 --- a/frontend/src/app/main/ui/components/dropdown.cljs +++ b/frontend/src/app/main/ui/components/dropdown.cljs @@ -11,6 +11,7 @@ [app.util.globals :as globals] [app.util.keyboard :as kbd] [app.util.timers :as tm] + [beicon.v2.core :as rx] [goog.events :as events] [rumext.v2 :as mf]) (:import goog.events.EventType)) @@ -45,9 +46,10 @@ (fn [] (let [keys [(events/listen globals/document EventType.CLICK on-click) (events/listen globals/document EventType.CONTEXTMENU on-click) - (events/listen globals/document EventType.KEYUP on-keyup)]] - (tm/schedule #(mf/set-ref-val! listening-ref true)) - #(run! events/unlistenByKey keys)))] + (events/listen globals/document EventType.KEYUP on-keyup)] + timer (tm/schedule #(mf/set-ref-val! listening-ref true))] + #(do (rx/dispose! timer) + (run! events/unlistenByKey keys))))] (mf/use-effect on-mount) children)) diff --git a/frontend/src/app/main/ui/dashboard/sidebar.cljs b/frontend/src/app/main/ui/dashboard/sidebar.cljs index eeda604490..53d87a8878 100644 --- a/frontend/src/app/main/ui/dashboard/sidebar.cljs +++ b/frontend/src/app/main/ui/dashboard/sidebar.cljs @@ -40,6 +40,7 @@ [app.main.ui.ds.buttons.button :refer [button*]] [app.main.ui.ds.foundations.assets.icon :refer [icon*] :as i] [app.main.ui.ds.foundations.assets.raw-svg :refer [raw-svg*]] + [app.main.ui.hooks :refer [use-focus-timer-ref]] [app.main.ui.icons :as deprecated-icon] [app.main.ui.nitrate.nitrate-form] [app.util.dom :as dom] @@ -94,6 +95,14 @@ (def ^:private ^:svg-id penpot-logo-icon "penpot-logo-icon") (def ^:private ^:svg-id penpot-logo-icon-subtle "penpot-logo-subtle") +(defn schedule-focus-by-id! + [ref element-id] + (when-let [h (mf/ref-val ref)] + (ts/dispose! h)) + (mf/set-ref-val! ref + (ts/schedule + #(dom/focus-and-untabbable! (dom/get-element element-id))))) + (mf/defc sidebar-project* {::mf/private true} [{:keys [item is-selected]}] @@ -112,6 +121,8 @@ project-id (get item :id) + focus-timer-ref (use-focus-timer-ref) + on-click (mf/use-fn (mf/deps project-id) @@ -123,14 +134,9 @@ (mf/deps project-id) (fn [event] (when (kbd/enter? event) - (st/emit! - (dcm/go-to-dashboard-files :project-id project-id)) - (ts/schedule - (fn [] - (when-let [title (dom/get-element (str project-id))] - (dom/set-attribute! title "tabindex" "0") - (dom/focus! title) - (dom/set-attribute! title "tabindex" "-1"))))))) + (schedule-focus-by-id! focus-timer-ref (str project-id)) + (st/emit! (dcm/go-to-dashboard-files :project-id project-id))))) + on-menu-click (mf/use-fn @@ -228,6 +234,8 @@ focused? (mf/use-state false) emit! (mf/use-memo #(f/debounce st/emit! 500)) + focus-timer-ref (use-focus-timer-ref) + on-search-blur (mf/use-fn (fn [_] @@ -254,13 +262,7 @@ (mf/use-fn (fn [e] (when (kbd/enter? e) - (ts/schedule - (fn [] - (let [search-title (dom/get-element (str "dashboard-search-title"))] - (when search-title - (dom/set-attribute! search-title "tabindex" "0") - (dom/focus! search-title) - (dom/set-attribute! search-title "tabindex" "-1"))))) + (schedule-focus-by-id! focus-timer-ref "dashboard-search-title") (dom/prevent-default e) (dom/stop-propagation e)))) @@ -948,6 +950,8 @@ nitrate? (contains? cf/flags :nitrate) + focus-timer-ref (use-focus-timer-ref) + go-projects (mf/use-fn #(st/emit! (dcm/go-to-dashboard-recent))) @@ -957,12 +961,7 @@ (fn [] (st/emit! (dcm/go-to-dashboard-recent :team-id team-id)) - (ts/schedule - (fn [] - (when-let [projects-title (dom/get-element "dashboard-projects-title")] - (dom/set-attribute! projects-title "tabindex" "0") - (dom/focus! projects-title) - (dom/set-attribute! projects-title "tabindex" "-1")))))) + (schedule-focus-by-id! focus-timer-ref "dashboard-projects-title"))) go-fonts (mf/use-fn @@ -975,13 +974,7 @@ (fn [] (st/emit! (dcm/go-to-dashboard-fonts :team-id team-id)) - (ts/schedule - (fn [] - (let [font-title (dom/get-element "dashboard-fonts-title")] - (when font-title - (dom/set-attribute! font-title "tabindex" "0") - (dom/focus! font-title) - (dom/set-attribute! font-title "tabindex" "-1"))))))) + (schedule-focus-by-id! focus-timer-ref "dashboard-fonts-title"))) go-drafts (mf/use-fn @@ -994,12 +987,7 @@ (mf/deps team-id default-project-id) (fn [] (st/emit! (dcm/go-to-dashboard-files :team-id team-id :project-id default-project-id)) - (ts/schedule - (fn [] - (when-let [title (dom/get-element "dashboard-drafts-title")] - (dom/set-attribute! title "tabindex" "0") - (dom/focus! title) - (dom/set-attribute! title "tabindex" "-1")))))) + (schedule-focus-by-id! focus-timer-ref "dashboard-drafts-title"))) go-libs (mf/use-fn @@ -1012,13 +1000,7 @@ (fn [] (st/emit! (dcm/go-to-dashboard-libraries :team-id team-id)) - (ts/schedule - (fn [] - (let [libs-title (dom/get-element "dashboard-libraries-title")] - (when libs-title - (dom/set-attribute! libs-title "tabindex" "0") - (dom/focus! libs-title) - (dom/set-attribute! libs-title "tabindex" "-1"))))))) + (schedule-focus-by-id! focus-timer-ref "dashboard-libraries-title"))) pinned-projects (mf/with-memo [projects] diff --git a/frontend/src/app/main/ui/ds/tooltip/tooltip.cljs b/frontend/src/app/main/ui/ds/tooltip/tooltip.cljs index bc20c517c9..e3d6b153fb 100644 --- a/frontend/src/app/main/ui/ds/tooltip/tooltip.cljs +++ b/frontend/src/app/main/ui/ds/tooltip/tooltip.cljs @@ -322,25 +322,26 @@ (let [trigger-el (mf/ref-val trigger-ref) tooltip-el (mf/ref-val tooltip-ref)] (when (and trigger-el tooltip-el) - (ts/raf - (fn [] - (let [origin-brect (dom/get-bounding-rect trigger-el) - tooltip-brect (dom/get-bounding-rect tooltip-el) - window-size (dom/get-window-size)] - (when-let [[new-placement placement-rect] - (find-matching-placement - placement - tooltip-brect - origin-brect - window-size - offset)] - (dom/set-css-property! tooltip-el "inset-block-start" - (str (:top placement-rect) "px")) - (dom/set-css-property! tooltip-el "inset-inline-start" - (str (:left placement-rect) "px")) + (let [raf-id (ts/raf + (fn [] + (let [origin-brect (dom/get-bounding-rect trigger-el) + tooltip-brect (dom/get-bounding-rect tooltip-el) + window-size (dom/get-window-size)] + (when-let [[new-placement placement-rect] + (find-matching-placement + placement + tooltip-brect + origin-brect + window-size + offset)] + (dom/set-css-property! tooltip-el "inset-block-start" + (str (:top placement-rect) "px")) + (dom/set-css-property! tooltip-el "inset-inline-start" + (str (:left placement-rect) "px")) - (when (not= new-placement placement) - (reset! placement* new-placement))))))))))) + (when (not= new-placement placement) + (reset! placement* new-placement))))))] + #(ts/cancel-af! raf-id))))))) [:> :div props children diff --git a/frontend/src/app/main/ui/hooks.cljs b/frontend/src/app/main/ui/hooks.cljs index ae8ebd30d5..bb541dd8ea 100644 --- a/frontend/src/app/main/ui/hooks.cljs +++ b/frontend/src/app/main/ui/hooks.cljs @@ -281,6 +281,16 @@ (mf/set-ref-val! ref val)) (mf/ref-val ref))) +;; FIXME: replace with rumext +(defn use-focus-timer-ref + "Returns a ref for scheduling focus timers and disposes any pending + timer on component unmount." + [] + (let [ref (mf/use-ref nil)] + (mf/with-effect [] + #(some-> (mf/ref-val ref) ts/dispose!)) + ref)) + ;; FIXME: rename to use-focus-objects (defn with-focus-objects ([objects] diff --git a/frontend/src/app/util/dom.cljs b/frontend/src/app/util/dom.cljs index f4be22b6c9..d885e7771b 100644 --- a/frontend/src/app/util/dom.cljs +++ b/frontend/src/app/util/dom.cljs @@ -699,6 +699,13 @@ (when (some? node) (.setAttribute node attr value))) +(defn focus-and-untabbable! + [^js node] + (when (some? node) + (set-attribute! node "tabindex" "0") + (focus! node) + (set-attribute! node "tabindex" "-1"))) + (defn set-style! [^js node ^string style value] (when (some? node) From bbc7e9bee932c588e617cb31fe4b417379bcaea9 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 28 Jul 2026 12:47:33 +0200 Subject: [PATCH 06/15] :sparkles: Add a script for run ci-like tasks --- .gitignore | 1 + AGENTS.md | 1 + scripts/ci | 478 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 480 insertions(+) create mode 100755 scripts/ci diff --git a/.gitignore b/.gitignore index a0fb7f7f5a..76da22b35f 100644 --- a/.gitignore +++ b/.gitignore @@ -101,5 +101,6 @@ opencode.json /.opencode/plans /.opencode/reports /.opencode/prompts +/.ci-logs /.codex/ /tools/__pycache__ diff --git a/AGENTS.md b/AGENTS.md index b6aae1f32d..05284c1c73 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,4 +109,5 @@ precision while maintaining a strong focus on maintainability and performance. - `scripts/nrepl-eval.mjs` — Evaluate Clojure code via nREPL (backend + frontend). - `scripts/check-commit` — Validate commit messages against Penpot's commit guidelines. - `scripts/check-fmt-clj` — Check Clojure formatting without modifying files. +- `scripts/ci` — CI orchestration script for running lint, tests, and format checks across modules. See `scripts/ci --help`. diff --git a/scripts/ci b/scripts/ci new file mode 100755 index 0000000000..df693a9cd0 --- /dev/null +++ b/scripts/ci @@ -0,0 +1,478 @@ +#!/usr/bin/env bash +# scripts/ci - CI orchestration script for Penpot monorepo + +set -euo pipefail + +# Constants +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(dirname "$SCRIPT_DIR")" +LOG_DIR="$PROJECT_ROOT/.ci-logs" + +# Color codes +RED='\033[0;31m' +GREEN='\033[0;32m' +YELLOW='\033[1;33m' +BLUE='\033[0;34m' +CYAN='\033[0;36m' +BOLD='\033[1m' +NC='\033[0m' + +# Available modules +ALL_MODULES=("frontend" "backend" "common" "render-wasm" "exporter" "mcp" "plugins" "library") + +# Module commands +declare -A LINT_CMD=( + [frontend]="pnpm run lint:clj && pnpm run lint:js && pnpm run lint:scss" + [backend]="pnpm run lint" + [common]="pnpm run lint:clj" + [render-wasm]="./lint" + [exporter]="pnpm run lint" + [mcp]="" + [plugins]="pnpm run lint" + [library]="pnpm run lint" +) + +declare -A TEST_CMD=( + [frontend]="pnpm run test:quiet" + [backend]="clojure -M:dev:test" + [common]="clojure -M:dev:test && pnpm run test:quiet" + [render-wasm]="./test" + [exporter]="" + [mcp]="pnpm run test" + [plugins]="pnpm run test" + [library]="pnpm run test" +) + +declare -A FMT_CHECK_CMD=( + [frontend]="pnpm run check-fmt:clj && pnpm run check-fmt:js && pnpm run check-fmt:scss" + [backend]="pnpm run check-fmt" + [common]="pnpm run check-fmt:clj && pnpm run check-fmt:js" + [render-wasm]="cargo fmt --check" + [exporter]="pnpm run check-fmt" + [mcp]="pnpm run fmt:check" + [plugins]="pnpm run format:check" + [library]="pnpm run check-fmt" +) + +declare -A FMT_FIX_CMD=( + [frontend]="pnpm run fmt" + [backend]="pnpm run fmt" + [common]="pnpm run fmt:clj && pnpm run fmt:js" + [render-wasm]="cargo fmt" + [exporter]="pnpm run fmt" + [mcp]="pnpm run fmt" + [plugins]="pnpm run format" + [library]="pnpm run fmt" +) + +# Default options +MODULES=() +TASKS=("lint" "test" "fmt") +FIX_MODE=false +FAIL_FAST=false +VERBOSE=true +DRY_RUN=false + +# Results tracking +declare -A RESULTS=() +declare -a FAILED_LOGS=() + +timestamp() { + date "+%H:%M:%S" +} + +log_info() { + echo -e "${BLUE}[$(timestamp)]${NC} $1" +} + +log_success() { + echo -e "${GREEN}[$(timestamp)] ✓${NC} $1" +} + +log_error() { + echo -e "${RED}[$(timestamp)] ✗${NC} $1" +} + +log_warning() { + echo -e "${YELLOW}[$(timestamp)] ⚠${NC} $1" +} + +log_header() { + local module=$1 + local current=$2 + local total=$3 + echo "" + echo -e "${CYAN}━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}" + echo -e "${CYAN} ${BOLD}[$current/$total] $module${NC}" + echo -e "${CYAN}━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}" +} + +run_task() { + local module=$1 + local task=$2 + local cmd=$3 + local logfile="$LOG_DIR/${module}-${task}.log" + + if [[ -z "$cmd" ]]; then + log_warning "$task: not defined for $module, skipping" + RESULTS["$module:$task"]="SKIPPED" + return 0 + fi + + if [[ "$DRY_RUN" == "true" ]]; then + log_info "$task: would run '$cmd' in $module/" + RESULTS["$module:$task"]="DRY-RUN" + return 0 + fi + + log_info "Running ${BOLD}$task${NC} for $module..." + + mkdir -p "$LOG_DIR" + local start_time + start_time=$(date +%s) + + if [[ "$VERBOSE" == "true" ]]; then + echo -e "${BLUE} cmd: $cmd${NC}" + fi + + local exit_code=0 + (cd "$PROJECT_ROOT/$module" && eval "$cmd") > "$logfile" 2>&1 || exit_code=$? + + local end_time + end_time=$(date +%s) + local duration=$((end_time - start_time)) + + if [[ $exit_code -eq 0 ]]; then + log_success "$task passed for $module ${BLUE}(${duration}s)${NC}" + RESULTS["$module:$task"]="PASSED" + return 0 + else + log_error "$task failed for $module ${BLUE}(${duration}s)${NC}" + echo -e " ${RED}log: $logfile${NC}" + RESULTS["$module:$task"]="FAILED" + FAILED_LOGS+=("$logfile") + if [[ "$VERBOSE" == "true" ]]; then + echo -e "${YELLOW} --- last 30 lines ---${NC}" + tail -n 30 "$logfile" | sed 's/^/ /' + echo -e "${YELLOW} ---------------------${NC}" + fi + return 1 + fi +} + +run_module() { + local module=$1 + local failed=false + + for task in "${TASKS[@]}"; do + local cmd="" + case $task in + lint) + cmd="${LINT_CMD[$module]:-}" + ;; + test) + cmd="${TEST_CMD[$module]:-}" + ;; + fmt) + if [[ "$FIX_MODE" == "true" ]]; then + cmd="${FMT_FIX_CMD[$module]:-}" + else + cmd="${FMT_CHECK_CMD[$module]:-}" + fi + ;; + esac + + if ! run_task "$module" "$task" "$cmd"; then + failed=true + if [[ "$FAIL_FAST" == "true" ]]; then + return 1 + fi + fi + done + + if [[ "$failed" == "true" ]]; then + return 1 + fi + return 0 +} + +print_summary() { + echo "" + echo -e "${CYAN}━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}" + echo -e "${CYAN} ${BOLD}SUMMARY${NC}" + echo -e "${CYAN}━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}" + + local total=0 + local passed=0 + local failed=0 + local skipped=0 + + for module in "${MODULES[@]}"; do + for task in "${TASKS[@]}"; do + local result="${RESULTS[$module:$task]:-N/A}" + total=$((total + 1)) + case $result in + PASSED) + passed=$((passed + 1)) + echo -e " ${GREEN}✓${NC} $module:$task" + ;; + FAILED) + failed=$((failed + 1)) + echo -e " ${RED}✗${NC} $module:$task" + ;; + SKIPPED) + skipped=$((skipped + 1)) + echo -e " ${YELLOW}○${NC} $module:$task" + ;; + DRY-RUN) + echo -e " ${BLUE}~${NC} $module:$task" + ;; + esac + done + done + + echo "" + echo -e " ${BOLD}Total:${NC} $total ${GREEN}Passed:${NC} $passed ${RED}Failed:${NC} $failed ${YELLOW}Skipped:${NC} $skipped" + + if [[ $failed -gt 0 ]]; then + echo "" + echo -e " ${RED}${BOLD}Failed logs:${NC}" + for logfile in "${FAILED_LOGS[@]}"; do + echo -e " ${RED}•${NC} $logfile" + done + return 1 + fi + return 0 +} + +validate_module() { + local mod=$1 + for valid in "${ALL_MODULES[@]}"; do + if [[ "$valid" == "$mod" ]]; then + return 0 + fi + done + return 1 +} + +usage() { + cat <&2 + echo "Run '$(basename "$0") --help' for usage." >&2 + exit 1 + ;; + *) + if validate_module "$1"; then + MODULES+=("$1") + else + echo -e "${RED}Error: Unknown module '$1'${NC}" >&2 + echo "Valid modules: ${ALL_MODULES[*]}" >&2 + exit 1 + fi + shift + ;; + esac + done + + # Clean empty entries from TASKS after removal + TASKS=("${TASKS[@]// /}") + TASKS=("${TASKS[@]/#/}") + local clean_tasks=() + for t in "${TASKS[@]}"; do + if [[ -n "$t" ]]; then + clean_tasks+=("$t") + fi + done + TASKS=("${clean_tasks[@]}") + + # Set modules + if [[ "$run_all" == "true" ]]; then + MODULES=("${ALL_MODULES[@]}") + fi + + # Apply exclusions + if [[ ${#exclude_modules[@]} -gt 0 ]]; then + local filtered=() + for mod in "${MODULES[@]}"; do + local excluded=false + for ex in "${exclude_modules[@]}"; do + if [[ "$mod" == "$ex" ]]; then + excluded=true + break + fi + done + if [[ "$excluded" == "false" ]]; then + filtered+=("$mod") + fi + done + MODULES=("${filtered[@]}") + fi + + # Validate + if [[ ${#MODULES[@]} -eq 0 ]]; then + echo -e "${RED}Error: No modules specified.${NC}" >&2 + echo "Use --all or specify modules: ${ALL_MODULES[*]}" >&2 + exit 1 + fi + + if [[ ${#TASKS[@]} -eq 0 ]]; then + echo -e "${RED}Error: No tasks selected.${NC}" >&2 + exit 1 + fi + + # Print configuration + echo "" + echo -e "${CYAN}${BOLD}Penpot CI Orchestrator${NC}" + echo -e "${CYAN}━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}" + echo -e " ${BOLD}Modules:${NC} ${MODULES[*]}" + echo -e " ${BOLD}Tasks:${NC} ${TASKS[*]}" + echo -e " ${BOLD}Fix mode:${NC} $FIX_MODE" + echo -e " ${BOLD}Fail-fast:${NC} $FAIL_FAST" + if [[ "$DRY_RUN" == "true" ]]; then + echo -e " ${BOLD}Mode:${NC} ${YELLOW}DRY RUN${NC}" + fi + echo "" + + # Run + local total_modules=${#MODULES[@]} + local current=0 + local modules_failed=0 + local start_time + start_time=$(date +%s) + + for module in "${MODULES[@]}"; do + current=$((current + 1)) + log_header "$module" "$current" "$total_modules" + + if ! run_module "$module"; then + modules_failed=$((modules_failed + 1)) + if [[ "$FAIL_FAST" == "true" ]]; then + log_error "Fail-fast: stopping due to failure in $module" + break + fi + fi + done + + local end_time + end_time=$(date +%s) + local total_duration=$((end_time - start_time)) + + print_summary || true + + echo "" + echo -e " ${BOLD}Duration:${NC} ${total_duration}s" + echo "" + + if [[ $modules_failed -gt 0 ]]; then + exit 1 + fi + exit 0 +} + +main "$@" From adda7e6645fcbe88532c04cd7c9906d52168dc02 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 29 Jul 2026 19:55:12 +0200 Subject: [PATCH 07/15] :books: Update testing serena memories --- .serena/memories/testing.md | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/.serena/memories/testing.md b/.serena/memories/testing.md index ce4990ba12..295da86212 100644 --- a/.serena/memories/testing.md +++ b/.serena/memories/testing.md @@ -137,17 +137,32 @@ E2E tests should not be added unless explicitly requested. ## Execution discipline -When running CLJS/JS tests (frontend, common): +**CRITICAL: Test output handling rules** +When running ANY test command (CLJS/JS or JVM): + +1. **NEVER pipe test output directly to `| head`, `| tail`, `| grep`, or similar filters** — this can hide failures and cause you to miss critical errors. +2. **ALWAYS pipe to a file first, then read the file:** + ```bash + # CORRECT: + pnpm run test 2>&1 > /tmp/test-output.txt + grep -A 5 "failures" /tmp/test-output.txt + + # WRONG: + pnpm run test 2>&1 | tail -20 + pnpm run test 2>&1 | grep "failures" + ``` +3. **Use `--focus` to narrow test scope** instead of filtering output. +4. **Read the full output file** to understand test results completely. + +When running CLJS/JS tests (frontend, common): - **Always use `pnpm run test:quiet`** — it silently builds the test bundle then runs the test runner, giving you clean test output. -- **Never pipe test output through `tail`, `head`, or similar filters** — doing so can silently hide test failures. Use `--focus` to narrow scope instead. -- **If you need to filter output, tee to a temp file first:** `pnpm run test:quiet 2>&1 | tee /tmp/penpot-test-output.txt`. The full output is preserved on disk so you can `grep`/`tail`/`head` the file without re-running. - Use `pnpm run test` when you want to see build output alongside test results (always builds, then runs). - After `build:test` has been run once, you can invoke the runner directly: `node target/tests/test.js [--focus ...] [--log-level ...]`. When running JVM tests (backend, common): - Use `clojure -M:dev:test` directly (no pnpm wrapper). -- The same no-piping rule applies: use `--focus` to narrow scope. +- Same file-piping rule applies. ## Verification Checklist From fadb3124a0051b76a47a5025f2a3f7d1dea2fc7b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 30 Jul 2026 12:49:00 +0200 Subject: [PATCH 08/15] :bug: Clamp gradient stop offsets to valid range (#10881) * :bug: Clamp gradient stop offsets to valid [0, 1] range Fixed a bug where gradient stop offsets outside the valid [0, 1] range were being sent to the server, causing schema validation errors ('invalid shape found'). Changes: - Viewport gradient handler: clamp offset in points-on-pointer-down before creating new stops - Colorpicker gradient preview: clamp offset in handle-preview-down before adding stops - Data layer: clamp offset parameter in update-colorpicker-add-stop and all stop offsets in update-colorpicker-stops; added app.common.math require All clamping follows the existing pattern used in handle-marker-pointer-move. AI-assisted-by: deepseek-v4-flash * :lipstick: Fix formatting in gradient handlers Fix cljfmt formatting issues in gradient handler functions. AI-assisted-by: qwen3.7-plus --- frontend/src/app/main/data/workspace/colors.cljs | 7 +++++-- frontend/src/app/main/store.cljs | 2 +- .../src/app/main/ui/workspace/colorpicker/gradients.cljs | 1 + frontend/src/app/main/ui/workspace/viewport/gradients.cljs | 1 + 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/main/data/workspace/colors.cljs b/frontend/src/app/main/data/workspace/colors.cljs index c145a203b7..c137936650 100644 --- a/frontend/src/app/main/data/workspace/colors.cljs +++ b/frontend/src/app/main/data/workspace/colors.cljs @@ -9,6 +9,7 @@ [app.common.data :as d] [app.common.data.macros :as dm] [app.common.files.helpers :as cfh] + [app.common.math :as mth] [app.common.schema :as sm] [app.common.types.color :as clr] [app.common.types.fills :as types.fills] @@ -950,7 +951,8 @@ (or (not cap-stops?) (< (count stops) types.fills/MAX-GRADIENT-STOPS))] (if can-add-stop? - (let [new-stop (-> (clr/interpolate-gradient stops offset) + (let [offset (mth/clamp offset 0 1) + new-stop (-> (clr/interpolate-gradient stops offset) (split-color-components)) stops (conj stops new-stop) stops (into [] (sort-by :offset stops)) @@ -973,7 +975,8 @@ stops (mapv split-color-components (if cap-stops? (take types.fills/MAX-GRADIENT-STOPS stops) - stops))] + stops)) + stops (mapv #(update % :offset (fn [o] (mth/clamp o 0 1))) stops)] (-> state (assoc :current-color (get stops stop)) (assoc :stops stops)))))))) diff --git a/frontend/src/app/main/store.cljs b/frontend/src/app/main/store.cljs index dc389033ae..8bdff34a65 100644 --- a/frontend/src/app/main/store.cljs +++ b/frontend/src/app/main/store.cljs @@ -121,7 +121,7 @@ delta (if prev-t (str "(+" (ct/diff-ms prev-t t) "ms)") "(+0ms)") - delta-pad (str/pad delta {:length 10 :type :right})] + delta-pad (str/pad delta {:length 10 :type :right})] (recur t (next xs) (conj! out (str iso " " delta-pad " " name)))) diff --git a/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs b/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs index 7010e84371..93648bc80d 100644 --- a/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs +++ b/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs @@ -233,6 +233,7 @@ (mf/deps on-add-stop-preview) (fn [^js e] (let [offset (-> (event->offset e) + (mth/clamp 0 1) (mth/precision 2))] (when on-add-stop-preview (on-add-stop-preview offset))))) diff --git a/frontend/src/app/main/ui/workspace/viewport/gradients.cljs b/frontend/src/app/main/ui/workspace/viewport/gradients.cljs index bc16b89045..c135d11e7e 100644 --- a/frontend/src/app/main/ui/workspace/viewport/gradients.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/gradients.cljs @@ -189,6 +189,7 @@ lv (-> (gpt/to-vec from-p to-p) (gpt/unit)) nv (gpt/normal-left lv) offset (-> (gsp/project-t position [from-p to-p] nv) + (mth/clamp 0 1) (mth/precision 2)) new-stop (cc/interpolate-gradient stops offset) stops (conj stops new-stop) From 040080749b94bb3ba466d937ce3eefb7697b8432 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 30 Jul 2026 12:49:19 +0200 Subject: [PATCH 09/15] :bug: Fix shape export failures when export name is nil or empty (#10852) Use cuerdas blank-name handling directly when normalizing frontend export payloads. Replace nil or blank export names with the object-id string in request-simple-export, request-multiple-export, clipboard export, and plugin direct export payloads so that the backend always receives a valid name. Add focused frontend tests for nil/blank name normalization and normalized request params. AI-assisted-by: nex-n2-pro --- .../src/app/main/data/exports/assets.cljs | 187 ++++++++++-------- .../app/main/data/workspace/clipboard.cljs | 21 +- frontend/src/app/plugins/shape.cljs | 15 +- .../data/exports_assets_test.cljs | 99 ++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 5 files changed, 223 insertions(+), 101 deletions(-) create mode 100644 frontend/test/frontend_tests/data/exports_assets_test.cljs diff --git a/frontend/src/app/main/data/exports/assets.cljs b/frontend/src/app/main/data/exports/assets.cljs index 24d72e5e92..b3adaebade 100644 --- a/frontend/src/app/main/data/exports/assets.cljs +++ b/frontend/src/app/main/data/exports/assets.cljs @@ -20,10 +20,27 @@ [app.util.dom :as dom] [app.util.websocket :as ws] [beicon.v2.core :as rx] + [cuerdas.core :as str] [potok.v2.core :as ptk])) (def default-timeout 5000) +(defn normalize-export + [{:keys [object-id name] :as export}] + (assoc export :name (if (str/blank? name) + (str object-id) + name))) + +(defn- normalize-exports + [exports] + (mapv normalize-export exports)) + +(defn- normalize-export-shapes-params + [{:keys [exports] :as params}] + (cond-> params + (seq exports) + (assoc :exports (normalize-exports exports)))) + (defn toggle-detail-visibililty [] (ptk/reify ::toggle-detail-visibililty @@ -181,104 +198,106 @@ (defn request-simple-export [{:keys [export]}] - (ptk/reify ::request-simple-export - ptk/UpdateEvent - (update [_ state] - (cond-> state - (not (use-wasm-export? state export)) - (update :export assoc :in-progress true :id uuid/zero))) + (let [export (normalize-export export)] + (ptk/reify ::request-simple-export + ptk/UpdateEvent + (update [_ state] + (cond-> state + (not (use-wasm-export? state export)) + (update :export assoc :in-progress true :id uuid/zero))) - ptk/WatchEvent - (watch [_ state _] - (if (use-wasm-export? state export) - (do - (case (:type export) - :pdf (wasm.exports/export-pdf export) - (wasm.exports/export-image export)) - (rx/empty)) - (let [profile-id (:profile-id state) - params {:exports [export] - :profile-id profile-id - :cmd :export-shapes - :wait true - :is-wasm (wasm-export-enabled? state)}] - (rx/concat - (dwp/force-persist-and-wait 400) + ptk/WatchEvent + (watch [_ state _] + (if (use-wasm-export? state export) + (do + (case (:type export) + :pdf (wasm.exports/export-pdf export) + (wasm.exports/export-image export)) + (rx/empty)) + (let [profile-id (:profile-id state) + params (normalize-export-shapes-params {:exports [export] + :profile-id profile-id + :cmd :export-shapes + :wait true + :is-wasm (wasm-export-enabled? state)})] + (rx/concat + (dwp/force-persist-and-wait 400) - (->> (rp/cmd! :export params) - (rx/map (fn [{:keys [filename mtype uri]}] - (dom/trigger-download-uri filename mtype uri) - (clear-export-state uuid/zero))) - (rx/catch (fn [cause] - (rx/concat - (rx/of (clear-export-state uuid/zero)) - (rx/throw cause))))))))))) + (->> (rp/cmd! :export params) + (rx/map (fn [{:keys [filename mtype uri]}] + (dom/trigger-download-uri filename mtype uri) + (clear-export-state uuid/zero))) + (rx/catch (fn [cause] + (rx/concat + (rx/of (clear-export-state uuid/zero)) + (rx/throw cause)))))))))))) (defn request-multiple-export [{:keys [exports cmd name] :or {cmd :export-shapes} :as params}] - (ptk/reify ::request-multiple-export - ptk/WatchEvent - (watch [_ state _] - (let [resource-id (volatile! nil) - profile-id (:profile-id state) - ws-conn (:ws-conn state) - params (cond-> - {:exports exports - :cmd cmd - :profile-id profile-id - :force-multiple true - :is-wasm (wasm-export-enabled? state)} - (some? name) - (assoc :name name)) + (let [exports (normalize-exports exports)] + (ptk/reify ::request-multiple-export + ptk/WatchEvent + (watch [_ state _] + (let [resource-id (volatile! nil) + profile-id (:profile-id state) + ws-conn (:ws-conn state) + params (cond-> + {:exports exports + :cmd cmd + :profile-id profile-id + :force-multiple true + :is-wasm (wasm-export-enabled? state)} + (some? name) + (assoc :name name)) - progress-stream - (->> (ws/get-rcv-stream ws-conn) - (rx/filter ws/message-event?) - (rx/map :payload) - (rx/filter #(= :export-update (:type %))) - (rx/filter #(= @resource-id (:resource-id %))) - (rx/share)) + progress-stream + (->> (ws/get-rcv-stream ws-conn) + (rx/filter ws/message-event?) + (rx/map :payload) + (rx/filter #(= :export-update (:type %))) + (rx/filter #(= @resource-id (:resource-id %))) + (rx/share)) - stopper - (rx/filter #(or (= "ended" (:status %)) - (= "error" (:status %))) - progress-stream)] + stopper + (rx/filter #(or (= "ended" (:status %)) + (= "error" (:status %))) + progress-stream)] - (swap! st/ongoing-tasks conj :export) + (swap! st/ongoing-tasks conj :export) - (rx/merge - ;; Force that all data is persisted; best effort. - (rx/of ::dwp/force-persist) + (rx/merge + ;; Force that all data is persisted; best effort. + (rx/of ::dwp/force-persist) - ;; Launch the exportation process and stores the resource id - ;; locally. - (->> (rp/cmd! :export params) - (rx/map (fn [{:keys [id] :as resource}] - (vreset! resource-id id) - (initialize-export-status exports cmd resource)))) + ;; Launch the exportation process and stores the resource id + ;; locally. + (->> (rp/cmd! :export params) + (rx/map (fn [{:keys [id] :as resource}] + (vreset! resource-id id) + (initialize-export-status exports cmd resource)))) - ;; We proceed to update the export state with incoming - ;; progress updates. We delay the stopper for give some time - ;; to update the status with ended or errored status before - ;; close the stream. - (->> progress-stream - (rx/map update-export-status) - (rx/take-until (rx/delay 500 stopper)) - (rx/finalize (fn [] - (swap! st/ongoing-tasks disj :export)))) + ;; We proceed to update the export state with incoming + ;; progress updates. We delay the stopper for give some time + ;; to update the status with ended or errored status before + ;; close the stream. + (->> progress-stream + (rx/map update-export-status) + (rx/take-until (rx/delay 500 stopper)) + (rx/finalize (fn [] + (swap! st/ongoing-tasks disj :export)))) - ;; We hide need to hide the ui elements of the export after - ;; some interval. We also delay a little bit more the stopper - ;; for ensure that after some security time, the stream is - ;; completely closed. - (->> progress-stream - (rx/filter #(= "ended" (:status %))) - (rx/take 1) - (rx/delay default-timeout) - (rx/map #(clear-export-state @resource-id)) - (rx/take-until (rx/delay 6000 stopper)))))))) + ;; We hide need to hide the ui elements of the export after + ;; some interval. We also delay a little bit more the stopper + ;; for ensure that after some security time, the stream is + ;; completely closed. + (->> progress-stream + (rx/filter #(= "ended" (:status %))) + (rx/take 1) + (rx/delay default-timeout) + (rx/map #(clear-export-state @resource-id)) + (rx/take-until (rx/delay 6000 stopper))))))))) (defn request-export [{:keys [exports] :as params}] diff --git a/frontend/src/app/main/data/workspace/clipboard.cljs b/frontend/src/app/main/data/workspace/clipboard.cljs index c87a5707f9..eca273832a 100644 --- a/frontend/src/app/main/data/workspace/clipboard.cljs +++ b/frontend/src/app/main/data/workspace/clipboard.cljs @@ -34,6 +34,7 @@ [app.config :as cf] [app.main.data.changes :as dch] [app.main.data.event :as ev] + [app.main.data.exports.assets :as de] [app.main.data.exports.wasm :as wasm.exports] [app.main.data.helpers :as dsh] [app.main.data.notifications :as ntf] @@ -1147,16 +1148,16 @@ page-id (:current-page-id state) selected (first (dsh/lookup-selected state)) - export {:file-id file-id - :page-id page-id - :object-id selected - ;; webp would be preferrable, but PNG is the most supported image MIME type by clipboard APIs. - :type :png - ;; Always use 2 to ensure good enough quality for wireframes. - :scale 2 - :suffix "" - :enabled true - :name ""} + export (de/normalize-export {:file-id file-id + :page-id page-id + :object-id selected + ;; webp would be preferrable, but PNG is the most supported image MIME type by clipboard APIs. + :type :png + ;; Always use 2 to ensure good enough quality for wireframes. + :scale 2 + :suffix "" + :enabled true + :name ""}) ;; Create a deferred promise immediately, before any async operations. ;; Registering the clipboard write NOW preserves the user-gesture security diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 4a1ea9bdf9..725f69c3c8 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -34,6 +34,7 @@ [app.common.types.text :as txt] [app.common.uuid :as uuid] [app.config :as cf] + [app.main.data.exports.assets :as de] [app.main.data.exports.wasm :as wasm.exports] [app.main.data.persistence :as dwp] [app.main.data.plugins :as dp] @@ -1547,13 +1548,13 @@ :profile-id (:profile-id @st/state) :wait true :is-wasm false - :exports [{:file-id file-id - :page-id page-id - :object-id id - :name (:name shape) - :type (:type value :png) - :suffix (:suffix value "") - :scale (:scale value 1)}]}] + :exports [(de/normalize-export {:file-id file-id + :page-id page-id + :object-id id + :name (:name shape) + :type (:type value :png) + :suffix (:suffix value "") + :scale (:scale value 1)})]}] (js/Promise. (fn [resolve reject] ;; The exporter renders the file from its persisted diff --git a/frontend/test/frontend_tests/data/exports_assets_test.cljs b/frontend/test/frontend_tests/data/exports_assets_test.cljs new file mode 100644 index 0000000000..d4ae9edea8 --- /dev/null +++ b/frontend/test/frontend_tests/data/exports_assets_test.cljs @@ -0,0 +1,99 @@ +;; 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.data.exports-assets-test + (:require + [app.common.uuid :as uuid] + [app.main.data.exports.assets :as de] + [app.main.data.persistence :as dwp] + [app.main.repo :as repo] + [app.main.store :as st] + [app.util.dom :as dom] + [app.util.websocket :as ws] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.events :as the] + [frontend-tests.helpers.mock :as mock] + [potok.v2.core :as ptk])) + +(def ^:private export {:id (uuid/next) + :object-id (uuid/next) + :type :png + :suffix "" + :scale 1}) + +(defn- export-with-name + [name] + (merge export {:name name})) + +(defn- test-state + [] + {:profile-id (:id export) + :ws-conn nil}) + +(t/deftest normalize-export-preserves-existing-name + (t/is (= (export-with-name "Layer 1") + (de/normalize-export (export-with-name "Layer 1"))))) + +(t/deftest normalize-export-replaces-nil-name-with-object-id + (t/is (= (export-with-name (str (:object-id export))) + (de/normalize-export (assoc export :name nil))))) + +(t/deftest normalize-export-replaces-empty-name-with-object-id + (t/is (= (export-with-name (str (:object-id export))) + (de/normalize-export (assoc export :name ""))))) + +(t/deftest request-simple-export-sends-normalized-export + (t/async done + (let [export (export-with-name "") + observed (atom nil)] + (mock/with-mocks {repo/cmd! (mock/stub (fn [_ params] + (reset! observed params) + (rx/of {:filename "export.png" + :mtype "image/png" + :uri "blob:export"}))) + dwp/force-persist-and-wait (mock/stub (fn [_] (rx/of ::force-persisted))) + dom/trigger-download-uri (mock/stub (fn [& _] nil))} + (fn [done'] + (let [completed (fn [_state] + (t/is (= (export-with-name (str (:object-id export))) + (-> @observed :exports first))))] + (ptk/emit! (the/prepare-store (test-state) done' completed) + (de/request-simple-export {:export export}) + :the/end))) + done)))) + +(t/deftest request-multiple-export-sends-normalized-enabled-exports + (t/async done + (let [exports [{:id "enabled-1" + :object-id "enabled-1" + :shape {:id "enabled-1"} + :type :png + :suffix "" + :scale 1 + :enabled true + :name ""}] + observed (atom nil)] + (mock/with-mocks {repo/cmd! (mock/stub (fn [_ params] + (reset! observed params) + (rx/of {:id (:id export)}))) + ws/get-rcv-stream (mock/stub (fn [_] (rx/empty))) + st/ongoing-tasks (atom #{})} + (fn [done'] + (let [completed (fn [_state] + (t/is (= [{:id "enabled-1" + :object-id "enabled-1" + :shape {:id "enabled-1"} + :type :png + :suffix "" + :scale 1 + :enabled true + :name "enabled-1"}] + (:exports @observed))))] + (ptk/emit! (the/prepare-store (test-state) done' completed) + (de/request-multiple-export {:exports exports}) + :the/end))) + done)))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 450f6978bd..7972b4e039 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -7,6 +7,7 @@ [frontend-tests.basic-shapes-test] [frontend-tests.code-gen-style-test] [frontend-tests.copy-as-svg-test] + [frontend-tests.data.exports-assets-test] [frontend-tests.data.nitrate-test] [frontend-tests.data.repo-test] [frontend-tests.data.store-test] @@ -88,6 +89,7 @@ 'frontend-tests.data.nitrate-test 'frontend-tests.data.repo-test 'frontend-tests.data.store-test + 'frontend-tests.data.exports-assets-test 'frontend-tests.errors-test 'frontend-tests.main-errors-test 'frontend-tests.data.uploads-test From ef593514f231a91e206a067122b0325df5d3438d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 30 Jul 2026 12:50:46 +0200 Subject: [PATCH 10/15] :bug: Add nil guards on viewport-node in pixel overlay component (#10812) * :bug: Add nil guards on viewport-node in pixel overlay component Add nil checks for viewport-node in process-pointer-move, viewport->canvas-coords, process-pointer-move-wasm, pick-color-at-wasm, and handle-draw-picker-canvas. Fixes a crash ("can't access property 'getBoundingClientRect', ... is null") when the viewport DOM node is unmounted while the color picker eyedropper is active and pointer move events are still firing. Fixes #10811 AI-assisted-by: mimo-v2.5 * :bug: Remove unused app.common.pprint require from errors.cljs Fixes clj-kondo warning: namespace app.common.pprint is required but never used. AI-assisted-by: mimo-v2.5 --- .../ui/workspace/viewport/pixel_overlay.cljs | 98 ++++++++++--------- 1 file changed, 50 insertions(+), 48 deletions(-) diff --git a/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs b/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs index afc461b4c6..c756f06e52 100644 --- a/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs @@ -56,51 +56,52 @@ (defn process-pointer-move [viewport-node canvas canvas-image-data zoom-view-context last-picked-color client-x client-y] - (when-let [image-data (mf/ref-val canvas-image-data)] - (when-let [zoom-view-node (dom/get-element "picker-detail")] - (when-not (mf/ref-val zoom-view-context) - (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) - (let [canvas-width 260 - canvas-height 140 - {brx :left bry :top} (dom/get-bounding-rect viewport-node) + (when viewport-node + (when-let [image-data (mf/ref-val canvas-image-data)] + (when-let [zoom-view-node (dom/get-element "picker-detail")] + (when-not (mf/ref-val zoom-view-context) + (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) + (let [canvas-width 260 + canvas-height 140 + {brx :left bry :top} (dom/get-bounding-rect viewport-node) - x (mth/floor (- client-x brx)) - y (mth/floor (- client-y bry)) + x (mth/floor (- client-x brx)) + y (mth/floor (- client-y bry)) - img-width (unchecked-get image-data "width") - img-height (unchecked-get image-data "height") + img-width (unchecked-get image-data "width") + img-height (unchecked-get image-data "height") - zoom-context (mf/ref-val zoom-view-context) + zoom-context (mf/ref-val zoom-view-context) - sx (- x 32) - sy (if (cfg/check-browser? :safari) y (- y 17)) - sw 65 - sh 35 - dx 0 - dy 0 - dw canvas-width - dh canvas-height] + sx (- x 32) + sy (if (cfg/check-browser? :safari) y (- y 17)) + sw 65 + sh 35 + dx 0 + dy 0 + dw canvas-width + dh canvas-height] - (when (obj/get zoom-context "imageSmoothingEnabled") - (obj/set! zoom-context "imageSmoothingEnabled" false)) - (.clearRect zoom-context 0 0 canvas-width canvas-height) - (.drawImage zoom-context canvas sx sy sw sh dx dy dw dh) + (when (obj/get zoom-context "imageSmoothingEnabled") + (obj/set! zoom-context "imageSmoothingEnabled" false)) + (.clearRect zoom-context 0 0 canvas-width canvas-height) + (.drawImage zoom-context canvas sx sy sw sh dx dy dw dh) - ;; Only pick color when cursor is within canvas bounds to avoid garbage pixels - (when (and (>= x 0) (< x img-width) (>= y 0) (< y img-height)) - (let [offset (* (+ (* y img-width) x) 4) - rgba (unchecked-get image-data "data") - r (d/check-num (obj/get rgba (+ 0 offset)) 255) - g (d/check-num (obj/get rgba (+ 1 offset)) 255) - b (d/check-num (obj/get rgba (+ 2 offset)) 255) - a (d/check-num (obj/get rgba (+ 3 offset)) 255) - color [r g b a]] - ;; Store latest color synchronously so the click handler always reads - ;; the correct pixel even before the rAF fires (fixes race condition) - (mf/set-ref-val! last-picked-color color) - (timers/raf - (fn [] - (st/emit! (dwc/pick-color color)))))))))) + ;; Only pick color when cursor is within canvas bounds to avoid garbage pixels + (when (and (>= x 0) (< x img-width) (>= y 0) (< y img-height)) + (let [offset (* (+ (* y img-width) x) 4) + rgba (unchecked-get image-data "data") + r (d/check-num (obj/get rgba (+ 0 offset)) 255) + g (d/check-num (obj/get rgba (+ 1 offset)) 255) + b (d/check-num (obj/get rgba (+ 2 offset)) 255) + a (d/check-num (obj/get rgba (+ 3 offset)) 255) + color [r g b a]] + ;; Store latest color synchronously so the click handler always reads + ;; the correct pixel even before the rAF fires (fixes race condition) + (mf/set-ref-val! last-picked-color color) + (timers/raf + (fn [] + (st/emit! (dwc/pick-color color))))))))))) (mf/defc pixel-overlay* @@ -260,18 +261,19 @@ (defn- viewport->canvas-coords "Maps client (viewport) coordinates to device-pixel canvas coordinates." [viewport-node client-x client-y] - (let [{brx :left bry :top} (dom/get-bounding-rect viewport-node) - dpr (wasm.api/get-dpr) - x (mth/floor (- client-x brx)) - y (mth/floor (- client-y bry))] - [(mth/floor (* x dpr)) - (mth/floor (* y dpr))])) + (when viewport-node + (let [{brx :left bry :top} (dom/get-bounding-rect viewport-node) + dpr (wasm.api/get-dpr) + x (mth/floor (- client-x brx)) + y (mth/floor (- client-y bry))] + [(mth/floor (* x dpr)) + (mth/floor (* y dpr))]))) (defn process-pointer-move-wasm "Updates the magnifier loupe with the canvas region under the cursor. The actual color is only read on click (see `pick-color-at-wasm`)." [viewport-node canvas zoom-view-context client-x client-y] - (when canvas + (when (and canvas viewport-node) (when-let [zoom-view-node (dom/get-element "picker-detail")] (when-not (mf/ref-val zoom-view-context) (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) @@ -309,7 +311,7 @@ the correct color even on GPUs where a raw WebGL `readPixels` returned values with their byte order swapped." [viewport-node canvas client-x client-y] - (when canvas + (when (and canvas viewport-node) (let [[canvas-x canvas-y] (viewport->canvas-coords viewport-node client-x client-y) img-width (.-width canvas) img-height (.-height canvas)] @@ -370,7 +372,7 @@ handle-draw-picker-canvas (mf/use-callback (fn [] - (when canvas + (when (and canvas viewport-node) ;; Read current mouse position from ref so the loupe refreshes on ;; each render even without a mouse-move. (let [{mx :x my :y} (mf/ref-val initial-mouse-pos)] From 1ae9334064aba44d3765bc6ed09c6409cc630c08 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Thu, 30 Jul 2026 10:22:58 +0000 Subject: [PATCH 11/15] :bug: Fail fast on Redis task dispatch when no instance is connected #10958 In multi-user mode, plugin task requests are published to a Redis channel keyed by user token. When no MCP server instance held a plugin connection for that token (e.g. after the user navigated away from the workspace), the publish reached zero subscribers and the request was silently dropped, so every tool call stalled until the 30-second task timeout instead of failing with a meaningful error. RedisBridge.sendTaskRequest now returns the PUBLISH receiver count and releases its response-channel subscription when the request reached no receiver (or publishing failed), since no response can arrive. PluginBridge uses the count to reject the pending task immediately with the multi-user connection error message; publish failures likewise reject the task instead of surfacing as an unhandled rejection followed by a timeout. The pending- task settlement logic shared with the timeout handler is extracted into a rejectPendingTask helper. AI-assisted-by: claude-fable-5 --- mcp/packages/server/src/PluginBridge.ts | 66 ++++++++++++++++++++----- mcp/packages/server/src/RedisBridge.ts | 20 +++++++- 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index 1c24547b8f..23f6d09e15 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -20,6 +20,8 @@ interface ClientConnection { * over these connections. */ export class PluginBridge { + public static readonly MULTIUSER_CONNECTION_ERROR_MESSAGE = `No Penpot instance connected for user token. Please ensure that Penpot is connected and that the MCP client connection is using the correct token.`; + private readonly logger = createLogger("PluginBridge"); private readonly wsServer: WebSocketServer; private readonly connectedClients: Map = new Map(); @@ -189,6 +191,35 @@ export class PluginBridge { this.logger.info(`Task ${response.id} completed: success=${response.success}`); } + /** + * Rejects a still-pending task with the given error, releasing its correlation state. + * + * Clears the task's timeout (if armed) and removes the task from the pending-task + * index before rejecting its promise. Safe to call for a task that has already been + * settled (e.g. by a response or a timeout), in which case nothing happens. + * + * @param taskId - The ID of the task to reject + * @param error - The error with which to reject the task + * @returns Whether the task was still pending and has been rejected + */ + private rejectPendingTask(taskId: string, error: Error): boolean { + const pendingTask = this.pendingTasks.get(taskId); + if (!pendingTask) { + return false; + } + + const timeoutHandle = this.taskTimeouts.get(taskId); + if (timeoutHandle) { + clearTimeout(timeoutHandle); + this.taskTimeouts.delete(taskId); + } + this.pendingTasks.delete(taskId); + + pendingTask.rejectWithError(error); + this.logger.info(`Task ${taskId} rejected: ${error.message}`); + return true; + } + /** * Determines the client connection to use for executing a task. * @@ -207,9 +238,7 @@ export class PluginBridge { const connection = this.clientsByToken.get(sessionContext.userToken); if (!connection) { - throw new Error( - `No plugin instance connected for user token. Please ensure the plugin is running and connected with the correct token.` - ); + throw new Error(PluginBridge.MULTIUSER_CONNECTION_ERROR_MESSAGE); } return connection; @@ -257,6 +286,10 @@ export class PluginBridge { * `resolveWithResult`/`rejectWithError` methods. The same correlation and timeout * handling therefore applies regardless of the transport. * + * When routing via Redis, the task is rejected immediately (rather than timing out) + * if the published request reached no instance, i.e. if no instance holds a plugin + * connection for the session's user token, or if publishing fails outright. + * * @param task - The task to dispatch * @param useRedis - Whether to route the request via Redis (multi-instance) rather * than directly over the local WebSocket connection @@ -279,9 +312,17 @@ export class PluginBridge { // register the task for result correlation, then publish the request via Redis this.pendingTasks.set(task.id, task); - void redisBridge.sendTaskRequest(userToken, task.toRequest(), (response) => - this.handlePluginTaskResponse(response) - ); + void redisBridge + .sendTaskRequest(userToken, task.toRequest(), (response) => this.handlePluginTaskResponse(response)) + .then((receiverCount) => { + // fail fast when no instance received the request (no connection with matching user token in any instance) + if (receiverCount === 0) { + this.rejectPendingTask(task.id, new Error(PluginBridge.MULTIUSER_CONNECTION_ERROR_MESSAGE)); + } + }) + .catch((error) => { + this.rejectPendingTask(task.id, error instanceof Error ? error : new Error(String(error))); + }); // on timeout, release the response-channel subscription, since no response // will arrive to trigger its self-unsubscribe. @@ -300,14 +341,13 @@ export class PluginBridge { // Set up a timeout to reject the task if no response is received const timeoutHandle = setTimeout(() => { - const pendingTask = this.pendingTasks.get(task.id); - if (pendingTask) { - this.pendingTasks.delete(task.id); - this.taskTimeouts.delete(task.id); - onTimeout?.(); - pendingTask.rejectWithError( + if ( + this.rejectPendingTask( + task.id, new Error(`Task ${task.id} timed out after ${this.taskTimeoutSecs} seconds`) - ); + ) + ) { + onTimeout?.(); } }, this.taskTimeoutSecs * 1000); diff --git a/mcp/packages/server/src/RedisBridge.ts b/mcp/packages/server/src/RedisBridge.ts index d8117d9242..89c01a4871 100644 --- a/mcp/packages/server/src/RedisBridge.ts +++ b/mcp/packages/server/src/RedisBridge.ts @@ -91,12 +91,16 @@ export class RedisBridge { * @param userToken - The user token identifying the target plugin's request channel * @param request - The serialized plugin task request, passed through verbatim * @param onResponse - Handler invoked with the response when it arrives + * @returns The number of instances that received the request. A count of 0 means no + * instance is subscribed to the token's request channel (i.e. the plugin is not + * connected anywhere); the request was dropped, no response will ever arrive, and + * the response subscription has already been released. */ async sendTaskRequest( userToken: string, request: PluginTaskRequest, onResponse: TaskResponseHandler - ): Promise { + ): Promise { const responseChannel = this.responseChannel(request.id); const requestChannel = this.requestChannel(userToken); @@ -113,7 +117,19 @@ export class RedisBridge { await this.subscriber.subscribe(responseChannel); // publish only once the response subscription is confirmed - await this.publisher.publish(requestChannel, JSON.stringify(request)); + let receiverCount: number; + try { + receiverCount = await this.publisher.publish(requestChannel, JSON.stringify(request)); + } catch (error) { + // the request was never delivered, so no response can arrive + await this.unsubscribeFromResponse(request.id); + throw error; + } + if (receiverCount === 0) { + // no subscriber received the request, so no response can arrive + await this.unsubscribeFromResponse(request.id); + } + return receiverCount; } /** From b4659df5b28b473f8191a3f93748ca7a5dfd1adf Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Thu, 30 Jul 2026 10:40:33 +0000 Subject: [PATCH 12/15] :bug: Preserve established plugin connection when rejecting a duplicate #10961 In multi-user mode, rejecting a second plugin WebSocket connection for an already-registered user token performed the full removeConnection cleanup for the newcomer. Since the token-keyed cleanup is keyed by token rather than by socket, this deleted the clientsByToken entry and the Redis request-channel subscription of the established, healthy connection. That connection then remained open and heartbeating but was unroutable, so every subsequent MCP tool call for the user failed although a valid plugin connection existed. removeConnection now performs the token-keyed cleanup only if the removed connection actually owns the token registration, so rejecting a duplicate releases only the resources the newcomer itself registered. AI-assisted-by: claude-fable-5 --- mcp/packages/server/src/PluginBridge.ts | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index 23f6d09e15..e8dda27ac3 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -133,9 +133,10 @@ export class PluginBridge { /** * Removes a client connection and releases all resources associated with it. * - * Clears the per-connection keep-alive interval and removes the connection - * from both the socket-keyed and token-keyed indexes. Safe to call with a - * socket that is not (or no longer) registered. + * Clears the per-connection keep-alive interval and removes the connection from the + * socket-keyed index. The token-keyed index entry (and, in multi-instance mode, the + * token's Redis task subscription) is removed only if it is owned by the given + * connection. Safe to call with a socket that is not (or no longer) registered. * * @param ws - The WebSocket whose connection state should be removed */ @@ -147,12 +148,18 @@ export class PluginBridge { clearInterval(connection.pingInterval); this.connectedClients.delete(ws); if (connection.userToken) { - this.clientsByToken.delete(connection.userToken); + // Perform the token-keyed cleanup only if this connection owns the token registration. + // A connection rejected as a duplicate carries the same token but must not remove token associations. + if (this.clientsByToken.get(connection.userToken) !== connection) { + this.logger.debug("Removed connection does not own its token registration; skipping token cleanup"); + } else { + this.clientsByToken.delete(connection.userToken); - if (this.redisBridge) { - this.redisBridge - .unsubscribeFromTasks(connection.userToken) - .catch((error) => this.logger.error(error, "Failed to unsubscribe from Redis task channel")); + if (this.redisBridge) { + this.redisBridge + .unsubscribeFromTasks(connection.userToken) + .catch((error) => this.logger.error(error, "Failed to unsubscribe from Redis task channel")); + } } } } From 30943f1074515a7184f20d29e9f9e40b787e5025 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Thu, 30 Jul 2026 10:38:29 +0200 Subject: [PATCH 13/15] :sparkles: Make MCP tool call timeout configurable, raising default The timeout for tool calls (which is trictly relevant for plugin tasks only) is now configurable via env. var PENPOT_MCP_TOOL_TIMEOUT_S. The default was raised from 30 to 120, because 30 seconds was not enough for some calls, especially in larger Penpot files. #10953 --- mcp/README.md | 1 + mcp/packages/server/src/PenpotMcpServer.ts | 3 ++- mcp/packages/server/src/PluginBridge.ts | 6 ++++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/mcp/README.md b/mcp/README.md index 3efc6255e7..846ee4d128 100644 --- a/mcp/README.md +++ b/mcp/README.md @@ -272,6 +272,7 @@ The Penpot MCP server can be configured using environment variables. | `PENPOT_MCP_REPL_PORT` | Port for the REPL server (development/debugging) | `4403` | | `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. Set to `true` to enable. | `false` | +| `PENPOT_MCP_TOOL_TIMEOUT_S` | Timeout, in seconds, for tool calls dispatched to the Penpot plugin | `120` | | `PENPOT_MCP_EXPORT_SHAPE_MAX_PARALLEL_REQUESTS` | Maximum number of parallel export shape requests (multi-user mode only). | `0` (no limit) | | `PENPOT_MCP_REDIS_URI` | Redis connection URI (e.g. `redis://host:6379`) enabling multi-instance horizontal scaling via Redis pub/sub task routing (multi-user mode only). When unset, the server runs in single-instance mode, requiring the plugin and MCP client to connect to the same instance. | (unset) | diff --git a/mcp/packages/server/src/PenpotMcpServer.ts b/mcp/packages/server/src/PenpotMcpServer.ts index 8a4ee30f25..bd992ec108 100644 --- a/mcp/packages/server/src/PenpotMcpServer.ts +++ b/mcp/packages/server/src/PenpotMcpServer.ts @@ -127,6 +127,7 @@ export class PenpotMcpServer { this.webSocketPort = parseInt(process.env.PENPOT_MCP_WEBSOCKET_PORT ?? "4402", 10); 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); this.configLoader = new ConfigurationLoader(process.cwd()); this.apiDocs = new ApiDocs(); @@ -147,7 +148,7 @@ export class PenpotMcpServer { this.redisBridge = new RedisBridge(redisUri, this.tenant); } - this.pluginBridge = new PluginBridge(this, this.webSocketPort, this.redisBridge); + this.pluginBridge = new PluginBridge(this, this.webSocketPort, toolTimeoutSecs, this.redisBridge); this.replServer = new ReplServer(this.pluginBridge, this.replPort, this.host); } diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index e8dda27ac3..ac7a2c005c 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -24,6 +24,7 @@ export class PluginBridge { private readonly logger = createLogger("PluginBridge"); private readonly wsServer: WebSocketServer; + private readonly connectedClients: Map = new Map(); private readonly clientsByToken: Map = new Map(); private readonly pendingTasks: Map> = new Map(); @@ -39,12 +40,13 @@ export class PluginBridge { * holding the relevant plugin's WebSocket connection (which may be this same * instance) via Redis, rather than dispatched directly over a local socket. * @param taskTimeoutSecs - Timeout, in seconds, for plugin task execution + * (defaults to {@link DEFAULT_TASK_TIMEOUT_SECS}) */ constructor( public readonly mcpServer: PenpotMcpServer, private port: number, - private readonly redisBridge?: RedisBridge, - private taskTimeoutSecs: number = 30 + private readonly taskTimeoutSecs: number, + private readonly redisBridge?: RedisBridge ) { this.wsServer = new WebSocketServer({ port: port }); this.setupWebSocketHandlers(); From 764b62906b868f2d860b54fd73ea6b31f2624b33 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 31 Jul 2026 12:06:19 +0200 Subject: [PATCH 14/15] :bug: Handle unrecognized JSON escape sequences as malformed-json (#10808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :paperclip: Update serena documentation about creating-prs workflow * :bug: Handle unrecognized JSON escape sequences as malformed-json When clojure.data.json's read-escaped-char encounters an unrecognized escape sequence (e.g. a backslash followed by '}', or other case fall-throughs in the parser) in a JSON request body, it throws a bare IllegalArgumentException. Previously this fell through to the generic RuntimeException branch in wrap-parse-request's handle-error, which unwrapped and recurred without matching, eventually reaching the internal-error handler and producing HTTP 500 + an error report — even though the root cause was malformed client input, not a server bug. The fix converts any IllegalArgumentException raised in the JSON parse path into a `:validation`/`:malformed-json` error by raising a new ex-info (which is caught by the top-level error handler in `app.http/router-handler`). The result is an HTTP 400 response with a descriptive hint, and no error report is generated. This addresses ~10% of all error reports received. The new IAE branch is placed before the RuntimeException branch in the cond (since IllegalArgumentException IS-A RuntimeException) and uses the throw-style (ex/raise) to match the existing RequestTooBigException / EOFException branches. A comment above the handle-error cond documents why raising is intentional and is caught by the top-level app.http error handler, not by the per-route wrap-errors middleware. Test suite changes: - Extend the existing `DummyRequest` defrecord in `http_middleware_test.clj` from 2 fields to 12 fields, implementing every IRequest method, and add a private `make-dummy-request` constructor that accepts an options map with every key optional and sensible `:or` defaults. Future fields added to DummyRequest won't break existing call sites as long as the `:or` defaults are kept in sync. - Remove the now-redundant `JsonRequest` defrecord and migrate all 11 `->DummyRequest` call sites to `make-dummy-request`. - Add 6 new deftest cases: - parse-request-illegal-argument-exception: malformed JSON body (containing `\}`) is converted to `:malformed-json`. - parse-request-request-too-big-exception: RequestTooBigException is converted to `:request-body-too-large`. - parse-request-eof-exception: java.io.EOFException is converted to `:malformed-json`. - parse-request-runtime-exception-with-cause: a wrapped RuntimeException recurses on ex-cause and dispatches to the matching specific branch. - parse-request-runtime-exception-without-cause: a bare RuntimeException falls through to errors/handle, returning 500 with :type :server-error :code :unexpected. - parse-request-non-runtime-throwable: java.io.IOException (a non-RuntimeException Throwable) is handled by the dedicated handle-exception method, returning 500 with :code :io-exception. Together, the new tests cover all 6 branches of wrap-parse-request's handle-error cond. Refs #10804. AI-assisted-by: minimax-m3 --- .serena/memories/workflow/creating-prs.md | 2 +- backend/src/app/http/middleware.clj | 26 ++- .../backend_tests/http_middleware_test.clj | 206 ++++++++++++++++-- 3 files changed, 215 insertions(+), 19 deletions(-) diff --git a/.serena/memories/workflow/creating-prs.md b/.serena/memories/workflow/creating-prs.md index 00252a7661..13bde56149 100644 --- a/.serena/memories/workflow/creating-prs.md +++ b/.serena/memories/workflow/creating-prs.md @@ -30,7 +30,7 @@ See `mem:workflow/creating-commits` for emoji codes. Squash merge uses the PR ti Include concise sections covering: - what changed and why; -- related GitHub issues or Taiga stories (`Fixes #NNNN`, `Relates to #NNNN`, `Taiga #NNNN`); +- related GitHub issues or Taiga stories (`Closes #NNNN`, `Relates to #NNNN`, `Taiga #NNNN`); - screenshots or recordings for UI-visible changes; - testing performed and residual risk; - breaking changes or migration notes, if any. diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 81b1ef13ec..fa2faa8a55 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -65,12 +65,25 @@ :else request))) + ;; The specific-exception branches below (IAE, + ;; RequestTooBigException, EOFException) raise with + ;; `ex/raise` rather than calling `errors/handle` directly. + ;; This is intentional: the throw is caught by the + ;; top-level error handler in `app.http/router-handler` + ;; (`backend/src/app/http.clj`), which routes every + ;; uncaught exception through `errors/handle`. The + ;; per-route `wrap-errors` middleware in the route list + ;; is a defensive layer; correctness does not depend on + ;; it. Raising here keeps the cond uniform with the + ;; existing RequestTooBigException / EOFException + ;; branches. (handle-error [cause request] (cond - (instance? RuntimeException cause) - (if-let [cause (ex-cause cause)] - (handle-error cause request) - (errors/handle cause request)) + (instance? IllegalArgumentException cause) + (ex/raise :type :validation + :code :malformed-json + :hint (ex-message cause) + :cause cause) (instance? RequestTooBigException cause) (ex/raise :type :validation @@ -83,6 +96,11 @@ :hint (ex-message cause) :cause cause) + (instance? RuntimeException cause) + (if-let [cause (ex-cause cause)] + (handle-error cause request) + (errors/handle cause request)) + :else (errors/handle cause request)))] diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index f751d8aca6..bd986fc031 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -21,19 +21,73 @@ [clojure.test :as t] [mockery.core :refer [with-mocks]] [yetti.request :as yreq] - [yetti.response :as yres])) + [yetti.response :as yres]) + (:import + io.undertow.server.RequestTooBigException)) (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) -(defrecord DummyRequest [headers cookies] +(defrecord DummyRequest [headers cookies method body-stream + remote-addr server-name server-port + scheme protocol path query ssl-client-cert] yreq/IRequestCookies (get-cookie [_ name] {:value (get cookies name)}) yreq/IRequest (get-header [_ name] - (get headers name))) + (get headers name)) + (method [_] method) + (body [_] body-stream) + (path [_] path) + (query [_] query) + (server-port [_] server-port) + (server-name [_] server-name) + (remote-addr [_] remote-addr) + (ssl-client-cert [_] ssl-client-cert) + (scheme [_] scheme) + (protocol [_] protocol)) + +(defn- make-dummy-request + "Constructs a DummyRequest from an options map. Every key is + optional; missing values fall back to sensible defaults. New + fields added to DummyRequest won't break existing call sites + as long as this constructor keeps its `:or` defaults in sync. + + Recognized keys: + :headers — map of header name → value + :cookies — map of cookie name → value + :method — HTTP method keyword (default :get) + :body-stream — InputStream for the body (used directly) + :body-bytes — bytes or string for the body; wrapped in a + ByteArrayInputStream if :body-stream is not + given + :remote-addr — string (default \"127.0.0.1\") + :server-name — string (default \"test\") + :server-port — long (default 0) + :scheme — keyword (default :http) + :protocol — string (default \"HTTP/1.1\") + :path — string (default \"/test\") + :query — string or nil (default nil) + :ssl-client-cert — X509Certificate or nil (default nil)" + [{:keys [headers cookies method body-stream body-bytes + remote-addr server-name server-port scheme protocol + path query ssl-client-cert] + :or {headers {} cookies {} method :get + body-stream nil + remote-addr "127.0.0.1" server-name "test" server-port 0 + scheme :http protocol "HTTP/1.1" path "/test" query nil + ssl-client-cert nil}}] + (let [body-stream (or body-stream + (when body-bytes + (java.io.ByteArrayInputStream. + (if (string? body-bytes) + (.getBytes ^String body-bytes "UTF-8") + body-bytes))))] + (->DummyRequest headers cookies method body-stream + remote-addr server-name server-port + scheme protocol path query ssl-client-cert))) (t/deftest auth-middleware-1 (let [request (volatile! nil) @@ -41,11 +95,11 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {"authorization" "Token aaaa"} {})) + (handler (make-dummy-request {:headers {"authorization" "Token aaaa"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :token token-type)) @@ -58,10 +112,10 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {"authorization" "Bearer aaaa"} {})) + (handler (make-dummy-request {:headers {"authorization" "Bearer aaaa"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :bearer token-type)) @@ -74,10 +128,10 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {} {"auth-token" "foobar"})) + (handler (make-dummy-request {:cookies {"auth-token" "foobar"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :cookie token-type)) @@ -89,16 +143,16 @@ (fn [req] {::yres/status 200}) {:test1 "secret-key"})] - (let [response (handler (->DummyRequest {} {}))] + (let [response (handler (make-dummy-request {}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "secret-key2"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "secret-key"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "test1 secret-key"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))] (t/is (= 200 (::yres/status response)))))) (t/deftest access-token-authz @@ -209,7 +263,7 @@ :user-agent "user agent"}) (#'session/assign-token cfg)) - response (handler (->DummyRequest {} {"auth-token" (:token session)})) + response (handler (make-dummy-request {:cookies {"auth-token" (:token session)}})) {:keys [token claims] token-type :type} (get response ::http/auth-data)] @@ -220,3 +274,127 @@ (t/is (= "penpot" (:aud claims))) (t/is (= (:id session) (:sid claims))) (t/is (= (:id profile) (:uid claims))))) + +(t/deftest parse-request-illegal-argument-exception + ;; clojure.data.json raises IllegalArgumentException (case + ;; fall-through) on several kinds of malformed input. The + ;; parse-request middleware should convert any such IAE into a + ;; 400 :malformed-json validation error rather than letting it + ;; surface as a 500 internal error. Because the conversion is + ;; done by raising an ex-info (caught by the top-level error + ;; handler in app.http/router-handler), this test asserts on + ;; the ex-info thrown by wrap-parse-request directly. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] {::yres/status 200 ::yres/body :ok})) + ;; Body contains the bytes for: {"x": "\}"} -- a string + ;; value with a backslash followed by '}', which + ;; clojure.data.json v0.5.x cannot handle. + body (.getBytes "{\"x\": \"\\}\"}" "UTF-8") + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes body}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))) + (t/is (string? (-> ex ex-data :hint))))) + +(t/deftest parse-request-request-too-big-exception + ;; When RequestTooBigException is raised (e.g. the request body + ;; exceeded the configured size limit), the middleware should + ;; convert it to a 413 :request-body-too-large validation + ;; error. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (RequestTooBigException. "too large")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :request-body-too-large (-> ex ex-data :code))) + (t/is (string? (-> ex ex-data :hint))))) + +(t/deftest parse-request-eof-exception + ;; When java.io.EOFException is raised (e.g. the body stream + ;; was closed before the parser could read it), the middleware + ;; should convert it to a 400 :malformed-json validation error. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (java.io.EOFException. "stream closed")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))) + (t/is (string? (-> ex ex-data :hint))))) + +(t/deftest parse-request-runtime-exception-with-cause + ;; When a RuntimeException with a non-nil ex-cause is raised, + ;; the middleware should recurse on the cause and dispatch + ;; through the specific-exception branches. Here we wrap an + ;; IllegalArgumentException in a RuntimeException and verify + ;; it surfaces as :malformed-json. + (let [iae (IllegalArgumentException. "No matching clause: 99") + wrapped (doto (RuntimeException. "wrapped") + (.initCause iae)) + handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw wrapped))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))))) + +(t/deftest parse-request-runtime-exception-without-cause + ;; When a bare RuntimeException (no ex-cause) is raised, the + ;; middleware should fall through to errors/handle's :default + ;; path and return a 500 with :type :server-error :code + ;; :unexpected. This is the "true internal error" path. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (RuntimeException. "boom")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + response (handler request) + body (::yres/body response)] + (t/is (= 500 (::yres/status response))) + (t/is (= :server-error (:type body))) + (t/is (= :unexpected (:code body))) + (t/is (= "boom" (:hint body))))) + +(t/deftest parse-request-non-runtime-throwable + ;; When a non-RuntimeException Throwable is raised (e.g. an + ;; Error subclass or a non-RuntimeException checked-style + ;; exception), the middleware should fall through to the + ;; :else branch and call errors/handle. java.io.IOException + ;; has a dedicated handle-exception method that returns 500 + ;; with :code :io-exception. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (java.io.IOException. "network gone")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + response (handler request) + body (::yres/body response)] + (t/is (= 500 (::yres/status response))) + (t/is (= :server-error (:type body))) + (t/is (= :io-exception (:code body))) + (t/is (= "network gone" (:hint body))))) From d04cbf175e0a28ae74f7b960d31d8ece5e2b971c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 31 Jul 2026 12:51:35 +0200 Subject: [PATCH 15/15] :bug: Fix nil dereference crash during flex layout drag operations (#10845) Production crash where @(get bounds id) threw "No protocol method IDeref.-deref defined for type null" when a shape ID had no corresponding entry in the bounds map during layout calculations. Added defensive nil guards (when-let / when) to all unprotected bounds dereference sites: - flex_layout/bounds.cljc: layout-content-points (parent + child) and layout-content-bounds - grid_layout/bounds.cljc: layout-content-points and layout-content-bounds - min_size_layout.cljc: child-min-width grid branch (3 sites) and child-min-height grid branch Added 7 new tests in geom_bounds_layout_nil_test.cljc covering all nil-bounds edge cases for flex, grid, and min-size layout paths. Registered in runner.cljc. Closes #10843 AI-assisted-by: qwen3.7-plus --- .../geom/shapes/flex_layout/bounds.cljc | 117 +++++----- .../geom/shapes/grid_layout/bounds.cljc | 54 ++--- .../common/geom/shapes/min_size_layout.cljc | 28 ++- .../geom_bounds_layout_nil_test.cljc | 213 ++++++++++++++++++ common/test/common_tests/runner.cljc | 2 + 5 files changed, 321 insertions(+), 93 deletions(-) create mode 100644 common/test/common_tests/geom_bounds_layout_nil_test.cljc diff --git a/common/src/app/common/geom/shapes/flex_layout/bounds.cljc b/common/src/app/common/geom/shapes/flex_layout/bounds.cljc index e6aef398f0..b434d99fc9 100644 --- a/common/src/app/common/geom/shapes/flex_layout/bounds.cljc +++ b/common/src/app/common/geom/shapes/flex_layout/bounds.cljc @@ -121,74 +121,79 @@ (defn layout-content-points [bounds parent children objects] - (let [parent-id (dm/get-prop parent :id) - parent-bounds @(get bounds parent-id) - reverse? (ctl/reverse? parent) - children (cond->> children (not reverse?) reverse)] + (let [parent-id (dm/get-prop parent :id) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [reverse? (ctl/reverse? parent) + children (cond->> children (not reverse?) reverse)] - (loop [children (seq children) - result (transient []) - correct-v (gpt/point 0)] + (loop [children (seq children) + result (transient []) + correct-v (gpt/point 0)] - (if (not children) - (persistent! result) + (if (not children) + (persistent! result) - (let [child (first children) - child-id (dm/get-prop child :id) - child-bounds @(get bounds child-id) - [margin-top margin-right margin-bottom margin-left] (ctl/child-margins child) + (let [child (first children) + child-id (dm/get-prop child :id) + child-bounds-ref (get bounds child-id) + child-bounds (some-> child-bounds-ref deref) + [margin-top margin-right margin-bottom margin-left] (ctl/child-margins child) - [child-bounds correct-v] - (if (or (ctl/fill-width? child) (ctl/fill-height? child)) - (child-layout-bound-points parent child parent-bounds child-bounds correct-v bounds objects) - [(->> child-bounds (map #(gpt/add % correct-v))) correct-v]) + [child-bounds correct-v] + (if (and child-bounds + (or (ctl/fill-width? child) (ctl/fill-height? child))) + (child-layout-bound-points parent child parent-bounds child-bounds correct-v bounds objects) + [(when child-bounds + (->> child-bounds (map #(gpt/add % correct-v)))) + correct-v]) - child-bounds - (when (d/not-empty? child-bounds) - (-> (gpo/parent-coords-bounds child-bounds parent-bounds) - (gpo/pad-points (- margin-top) (- margin-right) (- margin-bottom) (- margin-left))))] + child-bounds + (when (d/not-empty? child-bounds) + (-> (gpo/parent-coords-bounds child-bounds parent-bounds) + (gpo/pad-points (- margin-top) (- margin-right) (- margin-bottom) (- margin-left))))] - (recur (next children) - (cond-> result (some? child-bounds) (conj! child-bounds)) - correct-v)))))) + (recur (next children) + (cond-> result (some? child-bounds) (conj! child-bounds)) + correct-v)))))))) (defn layout-content-bounds [bounds {:keys [layout-padding] :as parent} children objects] - (let [parent-id (:id parent) - parent-bounds @(get bounds parent-id) + (let [parent-id (:id parent) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [row? (ctl/row? parent) + col? (ctl/col? parent) + space-around? (ctl/space-around? parent) + space-evenly? (ctl/space-evenly? parent) + content-evenly? (ctl/content-evenly? parent) + [layout-gap-row layout-gap-col] (ctl/gaps parent) - row? (ctl/row? parent) - col? (ctl/col? parent) - space-around? (ctl/space-around? parent) - space-evenly? (ctl/space-evenly? parent) - content-evenly? (ctl/content-evenly? parent) - [layout-gap-row layout-gap-col] (ctl/gaps parent) + row-pad (if (or (and col? space-evenly?) + (and col? space-around?) + (and row? content-evenly?)) + layout-gap-row + 0) - row-pad (if (or (and col? space-evenly?) - (and col? space-around?) - (and row? content-evenly?)) - layout-gap-row - 0) + col-pad (if (or (and row? space-evenly?) + (and row? space-around?) + (and col? content-evenly?)) + layout-gap-col + 0) - col-pad (if (or (and row? space-evenly?) - (and row? space-around?) - (and col? content-evenly?)) - layout-gap-col - 0) + {pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding + pad-top (+ (or pad-top 0) row-pad) + pad-right (+ (or pad-right 0) col-pad) + pad-bottom (+ (or pad-bottom 0) row-pad) + pad-left (+ (or pad-left 0) col-pad) - {pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding - pad-top (+ (or pad-top 0) row-pad) - pad-right (+ (or pad-right 0) col-pad) - pad-bottom (+ (or pad-bottom 0) row-pad) - pad-left (+ (or pad-left 0) col-pad) + layout-points + (layout-content-points bounds parent children objects)] - layout-points - (layout-content-points bounds parent children objects)] - - (if (d/not-empty? layout-points) - (-> layout-points - (gpo/merge-parent-coords-bounds parent-bounds) - (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) - ;; Cannot create some bounds from the children so we return the parent's - parent-bounds))) + (if (d/not-empty? layout-points) + (-> layout-points + (gpo/merge-parent-coords-bounds parent-bounds) + (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) + ;; Cannot create some bounds from the children so we return the parent's + parent-bounds))))) diff --git a/common/src/app/common/geom/shapes/grid_layout/bounds.cljc b/common/src/app/common/geom/shapes/grid_layout/bounds.cljc index 0628cf2060..caadff4766 100644 --- a/common/src/app/common/geom/shapes/grid_layout/bounds.cljc +++ b/common/src/app/common/geom/shapes/grid_layout/bounds.cljc @@ -12,36 +12,36 @@ (defn layout-content-points [bounds parent {:keys [row-tracks column-tracks]}] - (let [parent-id (:id parent) - parent-bounds @(get bounds parent-id) - - hv #(gpo/start-hv parent-bounds %) - vv #(gpo/start-vv parent-bounds %)] - (d/concat-vec - (->> row-tracks - (mapcat #(vector (:start-p %) - (gpt/add (:start-p %) (vv (:size %)))))) - (->> column-tracks - (mapcat #(vector (:start-p %) - (gpt/add (:start-p %) (hv (:size %))))))))) + (let [parent-id (:id parent) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [hv #(gpo/start-hv parent-bounds %) + vv #(gpo/start-vv parent-bounds %)] + (d/concat-vec + (->> row-tracks + (mapcat #(vector (:start-p %) + (gpt/add (:start-p %) (vv (:size %)))))) + (->> column-tracks + (mapcat #(vector (:start-p %) + (gpt/add (:start-p %) (hv (:size %))))))))))) (defn layout-content-bounds [bounds {:keys [layout-padding] :as parent} layout-data] - (let [parent-id (:id parent) - parent-bounds @(get bounds parent-id) + (let [parent-id (:id parent) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [{pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding + pad-top (or pad-top 0) + pad-right (or pad-right 0) + pad-bottom (or pad-bottom 0) + pad-left (or pad-left 0) - {pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding - pad-top (or pad-top 0) - pad-right (or pad-right 0) - pad-bottom (or pad-bottom 0) - pad-left (or pad-left 0) + layout-points (layout-content-points bounds parent layout-data)] - layout-points (layout-content-points bounds parent layout-data)] - - (if (d/not-empty? layout-points) - (-> layout-points - (gpo/merge-parent-coords-bounds parent-bounds) - (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) - ;; Cannot create some bounds from the children so we return the parent's - parent-bounds))) + (if (d/not-empty? layout-points) + (-> layout-points + (gpo/merge-parent-coords-bounds parent-bounds) + (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) + ;; Cannot create some bounds from the children so we return the parent's + parent-bounds))))) diff --git a/common/src/app/common/geom/shapes/min_size_layout.cljc b/common/src/app/common/geom/shapes/min_size_layout.cljc index 89f17871e3..57375098e9 100644 --- a/common/src/app/common/geom/shapes/min_size_layout.cljc +++ b/common/src/app/common/geom/shapes/min_size_layout.cljc @@ -31,13 +31,17 @@ (and (ctl/fill-width? child) (ctl/grid-layout? child)) - (let [children - (->> (cfh/get-immediate-children objects (:id child)) - (remove ctl/position-absolute?) - (map #(vector @(get bounds (:id %)) %))) - layout-data (gd/calc-layout-data child @(get bounds (:id child)) children bounds objects true)] - (max (ctl/child-min-width child) - (gpo/width-points (gb/layout-content-bounds bounds child layout-data)))) + (let [child-bounds-ref (get bounds (:id child))] + (if child-bounds-ref + (let [children + (->> (cfh/get-immediate-children objects (:id child)) + (remove ctl/position-absolute?) + (keep #(when-let [b (get bounds (:id %))] + [@b %]))) + layout-data (gd/calc-layout-data child @child-bounds-ref children bounds objects true)] + (max (ctl/child-min-width child) + (gpo/width-points (gb/layout-content-bounds bounds child layout-data)))) + (ctl/child-min-width child))) (ctl/fill-width? child) (ctl/child-min-width child) @@ -63,11 +67,15 @@ (let [children (->> (cfh/get-immediate-children objects (dm/get-prop child :id)) (remove ctl/position-absolute?) - (map (fn [child] [@(get bounds (:id child)) child]))) + (keep (fn [c] + (when-let [b (get bounds (:id c))] + [@b c])))) layout-data (gd/calc-layout-data child (:points child) children bounds objects true) auto-bounds (gb/layout-content-bounds bounds child layout-data)] - (max (ctl/child-min-height child) - (gpo/height-points auto-bounds))) + (if auto-bounds + (max (ctl/child-min-height child) + (gpo/height-points auto-bounds)) + (ctl/child-min-height child))) (ctl/fill-height? child) (ctl/child-min-height child) diff --git a/common/test/common_tests/geom_bounds_layout_nil_test.cljc b/common/test/common_tests/geom_bounds_layout_nil_test.cljc new file mode 100644 index 0000000000..db070ce4f4 --- /dev/null +++ b/common/test/common_tests/geom_bounds_layout_nil_test.cljc @@ -0,0 +1,213 @@ +;; 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 common-tests.geom-bounds-layout-nil-test + (:require + [app.common.data :as d] + [app.common.geom.bounds-map :as gbm] + [app.common.geom.point :as gpt] + [app.common.geom.rect :as grc] + [app.common.geom.shapes.flex-layout.bounds :as fb] + [app.common.geom.shapes.grid-layout.bounds :as gb] + [app.common.geom.shapes.min-size-layout :as msl] + [app.common.types.shape :as cts] + [app.common.types.shape.layout :as ctl] + [app.common.uuid :as uuid] + [clojure.test :as t])) + +;; ---- Helpers ---- + +(defn- make-rect + [id x y w h] + (-> (cts/setup-shape {:id id + :type :rect + :name (str "rect-" id) + :x x :y y :width w :height h}) + (assoc :parent-id uuid/zero + :frame-id uuid/zero))) + +(defn- make-flex-frame + [id child-ids & {:keys [x y w h dir] + :or {x 0 y 0 w 200 h 200 dir :row}}] + (-> (cts/setup-shape {:id id + :type :frame + :name (str "flex-" id) + :layout :flex + :layout-flex-dir dir + :x x :y y :width w :height h}) + (assoc :parent-id uuid/zero + :frame-id uuid/zero + :shapes (vec child-ids)))) + +(defn- make-grid-frame + [id child-ids & {:keys [x y w h dir] + :or {x 0 y 0 w 200 h 200 dir :row}}] + (let [cell-id (uuid/next)] + (-> (cts/setup-shape {:id id + :type :frame + :name (str "grid-" id) + :layout :grid + :layout-grid-dir dir + :layout-grid-columns [{:type :flex :value 1}] + :layout-grid-rows [{:type :flex :value 1}] + :layout-grid-cells + {cell-id {:id cell-id + :row 1 + :row-span 1 + :column 1 + :column-span 1 + :shapes (vec child-ids)}} + :layout-padding-type :multiple + :layout-padding {:p1 0 :p2 0 :p3 0 :p4 0} + :layout-gap {:column-gap 0 :row-gap 0} + :x x :y y :width w :height h}) + (assoc :parent-id uuid/zero + :frame-id uuid/zero + :shapes (vec child-ids))))) + +(defn- make-objects + [shapes] + (let [shape-map (into {} (map (fn [s] [(:id s) s]) shapes))] + (reduce-kv (fn [m _id shape] + (if (contains? shape :shapes) + (reduce (fn [m' child-id] + (assoc-in m' [child-id :parent-id] (:id shape))) + m + (:shapes shape)) + m)) + shape-map + shape-map))) + +(defn- bounds-map-from-objects + "Build a bounds map from objects, optionally excluding some IDs." + [objects & {:keys [exclude-ids]}] + (let [full (gbm/objects->bounds-map objects)] + (if (seq exclude-ids) + (apply dissoc full exclude-ids) + full))) + +;; ---- Tests for flex layout bounds with nil bounds ---- + +(t/deftest layout-content-points-with-missing-parent-bounds + (t/testing "layout-content-points returns nil when parent is not in bounds map" + (let [child-id (uuid/next) + parent-id (uuid/next) + child (make-rect child-id 10 10 50 50) + parent (make-flex-frame parent-id [child-id]) + objects (make-objects [parent child]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id})] + + (t/is (nil? (fb/layout-content-points bounds parent [child] objects)))))) + +(t/deftest layout-content-points-with-missing-child-bounds + (t/testing "layout-content-points skips children with missing bounds" + (let [child1-id (uuid/next) + child2-id (uuid/next) + parent-id (uuid/next) + child1 (make-rect child1-id 10 10 50 50) + child2 (make-rect child2-id 70 10 50 50) + parent (make-flex-frame parent-id [child1-id child2-id]) + objects (make-objects [parent child1 child2]) + bounds (bounds-map-from-objects objects :exclude-ids #{child1-id})] + + (let [result (fb/layout-content-points bounds parent [child1 child2] objects)] + (t/is (some? result)) + ;; Only child2's bounds should be in the result + (t/is (pos? (count result))))))) + +(t/deftest layout-content-bounds-with-missing-parent-bounds + (t/testing "layout-content-bounds returns nil when parent is not in bounds map" + (let [child-id (uuid/next) + parent-id (uuid/next) + child (make-rect child-id 10 10 50 50) + parent (make-flex-frame parent-id [child-id]) + objects (make-objects [parent child]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id})] + + (t/is (nil? (fb/layout-content-bounds bounds parent [child] objects)))))) + +;; ---- Tests for grid layout bounds with nil bounds ---- + +(t/deftest grid-layout-content-points-with-missing-parent-bounds + (t/testing "grid layout-content-points returns nil when parent is not in bounds map" + (let [parent-id (uuid/next) + parent (make-grid-frame parent-id []) + objects (make-objects [parent]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id}) + layout-data {:row-tracks [{:start-p (gpt/point 0 0) :size 100}] + :column-tracks [{:start-p (gpt/point 0 0) :size 100}]}] + + (t/is (nil? (gb/layout-content-points bounds parent layout-data)))))) + +(t/deftest grid-layout-content-bounds-with-missing-parent-bounds + (t/testing "grid layout-content-bounds returns nil when parent is not in bounds map" + (let [parent-id (uuid/next) + parent (make-grid-frame parent-id []) + objects (make-objects [parent]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id}) + layout-data {:row-tracks [{:start-p (gpt/point 0 0) :size 100}] + :column-tracks [{:start-p (gpt/point 0 0) :size 100}]}] + + (t/is (nil? (gb/layout-content-bounds bounds parent layout-data)))))) + +;; ---- Tests for min-size-layout with nil bounds ---- + +(t/deftest child-min-width-grid-with-missing-child-bounds + (t/testing "child-min-width falls back when grid layout child bounds are missing" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-grid-dir :row + :layout-item-h-sizing :fill)) + objects (make-objects [child grandchild]) + ;; Exclude grandchild from bounds to simulate missing entry + bounds (bounds-map-from-objects objects :exclude-ids #{grandchild-id}) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (let [result (msl/child-min-width child child-bounds bounds objects)] + (t/is (= (ctl/child-min-width child) result)))))) + +(t/deftest child-min-height-grid-with-missing-child-bounds + (t/testing "child-min-height falls back when grid layout child bounds are missing" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-grid-dir :column + :layout-item-v-sizing :fill)) + objects (make-objects [child grandchild]) + bounds (bounds-map-from-objects objects :exclude-ids #{grandchild-id}) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (let [result (msl/child-min-height child child-bounds bounds objects)] + (t/is (= (ctl/child-min-height child) result)))))) + +(t/deftest child-min-width-grid-with-present-child-bounds + (t/testing "child-min-width handles bounded children in a fill-width grid" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-item-h-sizing :fill)) + objects (make-objects [child grandchild]) + bounds (bounds-map-from-objects objects) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (t/is (number? (msl/child-min-width child child-bounds bounds objects)))))) + +(t/deftest child-min-height-grid-with-present-child-bounds + (t/testing "child-min-height handles bounded children in a fill-height grid" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-item-v-sizing :fill)) + objects (make-objects [child grandchild]) + bounds (bounds-map-from-objects objects) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (t/is (number? (msl/child-min-height child child-bounds bounds objects)))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 7738c5dd9e..d3d2f7d48e 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -22,6 +22,7 @@ [common-tests.files.shapes-builder-test] [common-tests.files.validate-test] [common-tests.geom-align-test] + [common-tests.geom-bounds-layout-nil-test] [common-tests.geom-bounds-map-test] [common-tests.geom-flex-layout-test] [common-tests.geom-grid-layout-test] @@ -95,6 +96,7 @@ 'common-tests.files-migrations-test 'common-tests.files.validate-test 'common-tests.geom-align-test + 'common-tests.geom-bounds-layout-nil-test 'common-tests.geom-bounds-map-test 'common-tests.geom-flex-layout-test 'common-tests.geom-grid-layout-test