From 367e4d534c536c33d4f3fbad375f3e9c29b787a6 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 14:36:29 +0200 Subject: [PATCH 01/19] :bug: Scope assemble-chunks session lookup to profile-id (#11012) Prevent BOLA in chunked upload assembly by verifying session ownership. The assemble-chunks function now requires a profile-id parameter and scopes the upload_session lookup accordingly, matching the pattern already used by upload-chunk. All three callers (assemble-file-media-object, create-font-variant, import-binfile) updated to pass the authenticated profile-id. AI-assisted-by: mimo-v2.5-pro --- backend/src/app/rpc/commands/binfile.clj | 2 +- backend/src/app/rpc/commands/fonts.clj | 4 +-- backend/src/app/rpc/commands/media.clj | 7 ++-- backend/test/backend_tests/rpc_media_test.clj | 35 +++++++++++++++++++ 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index 685b450ecb..ea26e5e2ce 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -155,7 +155,7 @@ params (if (some? upload-id) - (let [file (db/tx-run! cfg media-cmd/assemble-chunks upload-id)] + (let [file (db/tx-run! cfg media-cmd/assemble-chunks profile-id upload-id)] (assoc params :file file)) params) diff --git a/backend/src/app/rpc/commands/fonts.clj b/backend/src/app/rpc/commands/fonts.clj index c0ca2d8da7..7b5ac6ac4e 100644 --- a/backend/src/app/rpc/commands/fonts.clj +++ b/backend/src/app/rpc/commands/fonts.clj @@ -118,10 +118,10 @@ "Assembles each chunked-upload session in `uploads` (a `{mtype → session-id}` map) into a temp file, validates the media type and size of every entry, and returns a `{mtype → path}` data map." - [cfg {:keys [uploads] :as params}] + [cfg {:keys [::rpc/profile-id uploads] :as params}] (let [data (reduce-kv (fn [acc mtype session-id] - (let [assembled (assemble-chunks cfg session-id)] + (let [assembled (assemble-chunks cfg profile-id session-id)] (-> {:mtype mtype :size (:size assembled)} (media.v/validate-media-type! cm/font-types) (media.v/validate-font-size!)) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 612db21245..418eeb5b47 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -402,9 +402,10 @@ Raises a :validation/:missing-chunks error when the number of stored chunks does not match `:total-chunks` recorded in the session row. + Raises :not-found when the session does not belong to `profile-id`. Deletes the session row from `upload_session` on success." - [{:keys [::db/conn] :as cfg} session-id] - (let [session (db/get conn :upload-session {:id session-id}) + [{:keys [::db/conn] :as cfg} profile-id session-id] + (let [session (db/get conn :upload-session {:id session-id :profile-id profile-id}) chunks (get-upload-chunks conn session-id)] (when (not= (count chunks) (:total-chunks session)) @@ -447,7 +448,7 @@ (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] - (let [content (assemble-chunks cfg session-id) + (let [content (assemble-chunks cfg profile-id session-id) content (-> content (assoc :filename (str "upload:" name)) (assoc :mtype mtype) diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 4669ad929d..f7ac5dccf6 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -581,6 +581,41 @@ (t/is (some? (:error out))) (t/is (= :not-found (-> out :error ex-data :type))))) +(t/deftest chunked-upload-other-profile-cannot-assemble + ;; assemble-chunks must scope the session lookup to the requesting + ;; profile so that a different profile cannot assemble chunks from + ;; a session they do not own (BOLA / CWE-639). + (let [prof1 (th/create-profile* 1) + prof2 (th/create-profile* 2) + session-id (create-session! prof1 1) + source-path (th/tempfile "backend_tests/test_files/sample.jpg") + mfile {:filename "sample.jpg" + :path source-path + :mtype "image/jpeg" + :size 312043}] + + ;; prof1 uploads a chunk into their own session + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof1) + :session-id session-id + :index 0 + :content mfile})] + (t/is (nil? (:error out)))) + + ;; prof2 tries to assemble prof1's session via create-font-variant + ;; (which calls assemble-chunks without ownership check) + (let [out (th/command! {::th/type :create-font-variant + ::rpc/profile-id (:id prof2) + :team-id (:default-team-id prof2) + :font-id (uuid/next) + :font-family "TestFont" + :font-weight 400 + :font-style "normal" + :uploads {"font/ttf" session-id}})] + (t/is (some? (:error out))) + (t/is (= :not-found (-> out :error ex-data :type))) + (t/is (= :object-not-found (-> out :error ex-data :code)))))) + (t/deftest chunked-upload-invalid-media-type (let [prof (th/create-profile* 1) _ (th/create-project* 1 {:profile-id (:id prof) From 73c06688773f48356d0f5076177d3aaeee6bc494 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 14:36:50 +0200 Subject: [PATCH 02/19] :bug: Verify read access on source file in clone-file-media-object (#11090) The clone-file-media-object RPC command only checked edit permissions on the destination file. The source media object was fetched directly by UUID without verifying the caller had access to the file that owns it. This fix adds a read permission check on the source file before cloning. If the caller lacks read access to the source file, the operation fails with :not-found to avoid leaking information about the existence of files/media the caller cannot access. Closes #11087 AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/media.clj | 7 +- backend/test/backend_tests/rpc_media_test.clj | 95 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 418eeb5b47..4cf3c68029 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -272,8 +272,13 @@ (clone-file-media-object cfg params)) (defn clone-file-media-object - [{:keys [::db/conn]} {:keys [id file-id is-local]}] + [{:keys [::db/conn] :as cfg} {:keys [id file-id is-local] :as params}] (let [mobj (db/get-by-id conn :file-media-object id)] + (when-not mobj + (ex/raise :type :not-found + :code :object-not-found + :hint "source media object not found")) + (files/check-read-permissions! conn (::rpc/profile-id params) (:file-id mobj)) (db/insert! conn :file-media-object {:id (uuid/next) :file-id file-id diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index f7ac5dccf6..d230b03769 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -769,3 +769,98 @@ (t/is (some? (:error out))) (t/is (= :restriction (-> out :error ex-data :type))) (t/is (= :max-quote-reached (-> out :error ex-data :code))))))) + +;; --- Clone File Media Object BOLA tests --- + +(defn- create-storage-object! + [content content-type] + (let [storage (:app.storage/storage th/*system*)] + (sto/put-object! storage {::sto/content (sto/content content) + :content-type content-type}))) + +(t/deftest clone-file-media-object-success + (let [prof1 (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof1) + :team-id (:default-team-id prof1)}) + file1 (th/create-file* 1 {:profile-id (:id prof1) + :project-id (:default-project-id prof1) + :is-shared false}) + sobj (create-storage-object! "image-content" "image/png") + mobj (th/create-file-media-object* {:file-id (:id file1) + :name "test-media" + :width 100 + :height 100 + :mtype "image/png" + :media-id (:id sobj)}) + file2 (th/create-file* 2 {:profile-id (:id prof1) + :project-id (:default-project-id prof1) + :is-shared false}) + params {::th/type :clone-file-media-object + ::rpc/profile-id (:id prof1) + :file-id (:id file2) + :is-local true + :id (:id mobj)} + out (th/command! params)] + + (t/is (nil? (:error out))) + (let [result (:result out)] + (t/is (= (:id file2) (:file-id result))) + (t/is (= (:name mobj) (:name result))) + (t/is (= (:media-id mobj) (:media-id result))) + (t/is (uuid? (:id result))) + (t/is (not= (:id mobj) (:id result)))))) + +(t/deftest clone-file-media-object-no-read-access + (let [prof1 (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof1) + :team-id (:default-team-id prof1)}) + file1 (th/create-file* 1 {:profile-id (:id prof1) + :project-id (:default-project-id prof1) + :is-shared false}) + sobj (create-storage-object! "private-content" "image/png") + mobj (th/create-file-media-object* {:file-id (:id file1) + :name "private-media" + :width 100 + :height 100 + :mtype "image/png" + :media-id (:id sobj)}) + + prof2 (th/create-profile* 2) + _ (th/create-project* 2 {:profile-id (:id prof2) + :team-id (:default-team-id prof2)}) + file2 (th/create-file* 2 {:profile-id (:id prof2) + :project-id (:default-project-id prof2) + :is-shared false}) + + params {::th/type :clone-file-media-object + ::rpc/profile-id (:id prof2) + :file-id (:id file2) + :is-local true + :id (:id mobj)} + out (th/command! params)] + + (let [error (:error out) + error-data (ex-data error)] + (t/is (th/ex-info? error)) + (t/is (= :not-found (:type error-data))) + (t/is (= :object-not-found (:code error-data)))))) + +(t/deftest clone-file-media-object-source-not-found + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + params {::th/type :clone-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :id (uuid/random)} + out (th/command! params)] + + (let [error (:error out) + error-data (ex-data error)] + (t/is (th/ex-info? error)) + (t/is (= :not-found (:type error-data))) + (t/is (= :object-not-found (:code error-data)))))) From 3be07cccedc2c7259c8d8ef26e57c9c927b6448d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 14:37:51 +0200 Subject: [PATCH 03/19] :bug: Add minimum validation for total-chunks in upload session (#11104) The create-upload-session RPC method accepted total-chunks values of 0 or negative numbers without validation, creating inconsistent session state. Add {:min 1} constraint to the schema to reject invalid values at input validation. Closes #11103 AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/media.clj | 2 +- backend/test/backend_tests/rpc_media_test.clj | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 4cf3c68029..3dff04fa10 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -294,7 +294,7 @@ (def ^:private schema:create-upload-session [:map {:title "create-upload-session"} - [:total-chunks ::sm/int]]) + [:total-chunks [::sm/int {:min 1}]]]) (def ^:private schema:create-upload-session-result [:map {:title "create-upload-session-result"} diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index d230b03769..d22eabe64b 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -718,6 +718,24 @@ (t/is (= :max-quote-reached (-> out :error ex-data :code))) (t/is (= "upload-chunks-per-session" (-> out :error ex-data :target)))))) +(t/deftest chunked-upload-invalid-total-chunks + ;; total-chunks must be at least 1; zero and negative values are rejected + ;; with a :validation error. + (let [prof (th/create-profile* 1)] + ;; zero total-chunks + (let [out (th/command! {::th/type :create-upload-session + ::rpc/profile-id (:id prof) + :total-chunks 0})] + (t/is (some? (:error out))) + (t/is (= :validation (-> out :error ex-data :type)))) + + ;; negative total-chunks + (let [out (th/command! {::th/type :create-upload-session + ::rpc/profile-id (:id prof) + :total-chunks -1})] + (t/is (some? (:error out))) + (t/is (= :validation (-> out :error ex-data :type)))))) + (t/deftest chunked-upload-invalid-chunk-index ;; Both a negative index and an index >= total-chunks must be ;; rejected with a :validation / :invalid-chunk-index error. From e72c1869eb383240c059b3bac5c7c6026da6f313 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 15:13:49 +0200 Subject: [PATCH 04/19] :bug: Validate version parameter in import-binfile (#11107) Restrict version parameter to supported values (1 or 3) via schema validation instead of accepting any integer. Add content-based format detection when version is not provided, using bfc/parse-file-format to inspect file magic bytes. Closes #11105 AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/binfile.clj | 31 ++++++++++------ .../test/backend_tests/rpc_binfile_test.clj | 35 +++++++++++++++++-- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index ea26e5e2ce..ec4510200d 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -105,7 +105,11 @@ (try (case (int version) 1 (bf.v1/import-files! cfg) - 3 (bf.v3/import-files! cfg)) + 3 (bf.v3/import-files! cfg) + (throw (ex-info (str "Unsupported binfile version: " version) + {:type :validation + :code :unsupported-version + :version version}))) (finally (when owned? (fs/delete input-path))))] @@ -123,7 +127,7 @@ [:name [:or [:string {:max 250}] [:map-of ::sm/uuid [:string {:max 250}]]]] [:project-id ::sm/uuid] - [:version {:optional true} ::sm/int] + [:version {:optional true} [:enum 1 3]] [:file {:optional true} media.v/schema:upload] [:upload-id {:optional true} ::sm/uuid]] [:fn {:error/message "one of :file or :upload-id is required"} @@ -148,21 +152,28 @@ [:import-binfile/global]]} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id project-id version upload-id] :as params}] (projects/check-edition-permissions! pool profile-id project-id) - (let [version (or version 3) + (let [params (if (some? upload-id) + (let [file (db/tx-run! cfg media-cmd/assemble-chunks profile-id upload-id)] + (assoc params :file file)) + params) + + version (or version + (case (bfc/parse-file-format (-> params :file :path)) + :binfile-v1 1 + :binfile-v3 3)) + params (-> params (assoc :profile-id profile-id) (assoc :version version)) - params - (if (some? upload-id) - (let [file (db/tx-run! cfg media-cmd/assemble-chunks profile-id upload-id)] - (assoc params :file file)) - params) - manifest (case (int version) 1 nil - 3 (bf.v3/get-manifest (-> params :file :path)))] + 3 (bf.v3/get-manifest (-> params :file :path)) + (throw (ex-info (str "Unsupported binfile version: " version) + {:type :validation + :code :unsupported-version + :version version})))] (with-meta (sse/response (partial import-binfile cfg params)) diff --git a/backend/test/backend_tests/rpc_binfile_test.clj b/backend/test/backend_tests/rpc_binfile_test.clj index 536a980339..b1b99762b0 100644 --- a/backend/test/backend_tests/rpc_binfile_test.clj +++ b/backend/test/backend_tests/rpc_binfile_test.clj @@ -11,8 +11,7 @@ [app.rpc :as-alias rpc] [app.rpc.commands.binfile :as binfile] [backend-tests.helpers :as th] - [clojure.test :as t] - [datoteka.fs :as fs])) + [clojure.test :as t])) (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) @@ -32,3 +31,35 @@ (t/is (not (contains? (sm/keys (second schema)) :file-id)) "file-id should not be a declared parameter"))) + +(t/deftest import-binfile-schema-rejects-unsupported-version + ;; T1-N2-03: version parameter should be restricted to supported values (1 or 3) + (let [schema @#'binfile/schema:import-binfile + validator (sm/lazy-validator schema) + base-params {:name "test" + :project-id (uuid/random) + :upload-id (uuid/random)}] + + ;; Version 1 should be accepted + (t/is (true? (validator (assoc base-params :version 1))) + "version 1 should be valid") + + ;; Version 3 should be accepted + (t/is (true? (validator (assoc base-params :version 3))) + "version 3 should be valid") + + ;; Version 2 should be rejected + (t/is (false? (validator (assoc base-params :version 2))) + "version 2 should be rejected") + + ;; Version 0 should be rejected + (t/is (false? (validator (assoc base-params :version 0))) + "version 0 should be rejected") + + ;; Negative version should be rejected + (t/is (false? (validator (assoc base-params :version -1))) + "negative version should be rejected") + + ;; Version 4 should be rejected + (t/is (false? (validator (assoc base-params :version 4))) + "version 4 should be rejected"))) From 1671cc4fccda7bfd572111ae5b1a5a6f125206a5 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 17:43:25 +0200 Subject: [PATCH 05/19] :bug: Escape markdown in Mattermost error notifications (#11034) Add escape-markdown to common/data.cljc that escapes Markdown special characters (*, _, ~, `, [, ], >, #, @, etc.) by prefixing them with backslash. Apply it to user-controlled fields (:hint, :href) in the Mattermost error reporter before constructing the notification message. This is an internal-only feature not accessible to end users. AI-assisted-by: mimo-v2.5-pro --- backend/src/app/loggers/mattermost.clj | 5 +++-- common/src/app/common/data.cljc | 9 +++++++++ common/test/common_tests/data_test.cljc | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/backend/src/app/loggers/mattermost.clj b/backend/src/app/loggers/mattermost.clj index e3089f1f03..82a3c134e6 100644 --- a/backend/src/app/loggers/mattermost.clj +++ b/backend/src/app/loggers/mattermost.clj @@ -7,6 +7,7 @@ (ns app.loggers.mattermost "A mattermost integration for error reporting." (:require + [app.common.data :as d] [app.common.exceptions :as ex] [app.common.logging :as l] [app.common.pprint :as pp] @@ -25,7 +26,7 @@ (defn- send-mattermost-notification! [cfg {:keys [id] :as report}] (let [type (get report :type) - text (str "#" type " | " (get report :hint) "\n" + text (str "#" type " | " (d/escape-markdown (get report :hint)) "\n" (when id (str (u/join (cf/get :public-uri) "/dbg/error/" id) " ")) @@ -38,7 +39,7 @@ "- tenant: #" (:tenant report) "\n" "- origin: #" (:origin report) "\n" (when-let [href (get report :href)] - (str "- href: `" href "`\n")) + (str "- href: `" (d/escape-markdown href) "`\n")) (when-let [version (get report :frontend-version)] (str "- frontend-version: `" version "`\n")) (when-let [version (get report :backend-version)] diff --git a/common/src/app/common/data.cljc b/common/src/app/common/data.cljc index 418c6e5bd5..090fe8cd69 100644 --- a/common/src/app/common/data.cljc +++ b/common/src/app/common/data.cljc @@ -1192,6 +1192,15 @@ str/trim) "")) +(defn escape-markdown + "Escapes Markdown special characters by prefixing them with backslash. + Intended for user-controlled values embedded in Markdown messages + (e.g. Mattermost notifications)." + [s] + (if s + (str/replace (str s) #"([*_~`\[\]()>#+=\-|{}.!@\\])" (fn [[_ c]] (str "\\" c))) + "")) + (defn get-initials "Returns up to two uppercase initials extracted from a string. Non-letter prefixes in each token are ignored." diff --git a/common/test/common_tests/data_test.cljc b/common/test/common_tests/data_test.cljc index 46f12fd8fb..ffa26a2386 100644 --- a/common/test/common_tests/data_test.cljc +++ b/common/test/common_tests/data_test.cljc @@ -54,6 +54,25 @@ (t/is (= :keyword (d/normalize-string :keyword))) (t/is (= true (d/normalize-string true)))) +(t/deftest escape-markdown-test + (t/is (= "hello" (d/escape-markdown "hello"))) + (t/is (= "" (d/escape-markdown nil))) + (t/is (= "" (d/escape-markdown ""))) + (t/is (= "\\*bold\\*" (d/escape-markdown "*bold*"))) + (t/is (= "\\_italic\\_" (d/escape-markdown "_italic_"))) + (t/is (= "\\~strikethrough\\~" (d/escape-markdown "~strikethrough~"))) + (t/is (= "\\`code\\`" (d/escape-markdown "`code`"))) + (t/is (= "\\[link\\]\\(http://evil\\.com\\)" (d/escape-markdown "[link](http://evil.com)"))) + (t/is (= "\\> quote" (d/escape-markdown "> quote"))) + (t/is (= "\\# heading" (d/escape-markdown "# heading"))) + (t/is (= "\\@channel" (d/escape-markdown "@channel"))) + (t/is (= "\\!bang" (d/escape-markdown "!bang"))) + (t/is (= "normal\\-text" (d/escape-markdown "normal-text"))) + (t/is (= "a\\+b\\=c" (d/escape-markdown "a+b=c"))) + (t/is (= "pipe\\|separated" (d/escape-markdown "pipe|separated"))) + (t/is (= "curly\\{\\}braces" (d/escape-markdown "curly{}braces"))) + (t/is (= "backslash\\\\slash" (d/escape-markdown "backslash\\slash")))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Ordered Data Structures ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; From 4ac14cfd0808041037c77c3da9487240803ff71b Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Tue, 18 Aug 2026 17:45:42 +0200 Subject: [PATCH 06/19] :sparkles: Add component synchronization to waitForLayoutUpdate (#10964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :sparkles: Add component synchronization to waitForLayoutUpdate * :bug: Fix async mock leak in workspace-reflow-test Use mock/with-mocks instead of with-redefs for http/send! mock in failed-google-font-css-does-not-abort-shared-consumers test. with-redefs restores bindings when the block exits synchronously, but the RxJS subscription fires asynchronously. This caused the mock to leak into subsequent tests (workspace-media-test), producing 3 spurious failures. AI-assisted-by: mimo-v2.5-pro * :recycle: Replace async with-redefs with mock/with-mocks in frontend tests with-redefs restores bindings when the block exits synchronously, which is too early for async code (t/async, rx/subs!, promises). mock/with-mocks uses set! and restores in the done callback, keeping mocks alive across async boundaries. Converted 15 with-redefs usages across 4 test files: - workspace_reflow_test.cljs: 2 genuinely async tests (P1) - routes_test.cljs: 3 SSO caching tests (P2) - main_errors_test.cljs: 8 expired-org SSO tests (P2) - comments_test.cljs: 2 comment thread tests (P2) 36 purely sync with-redefs usages left unchanged — with-redefs is correct for synchronous code. AI-assisted-by: mimo-v2.5-pro * :paperclip: Fix fmt issues --------- Co-authored-by: Andrey Antukh --- .../app/main/data/workspace/libraries.cljs | 166 +++++---- frontend/src/app/main/data/workspace/mcp.cljs | 5 +- .../src/app/main/data/workspace/reflow.cljs | 195 +++++++---- .../main/data/workspace/reflow/signals.cljs | 141 ++++++++ .../app/main/data/workspace/selection.cljs | 19 + .../app/main/data/workspace/shape_layout.cljs | 5 +- .../src/app/main/data/workspace/shapes.cljs | 140 +++----- .../src/app/main/data/workspace/texts.cljs | 150 +++++--- .../app/main/data/workspace/wasm_text.cljs | 38 +- frontend/src/app/main/fonts.cljs | 51 ++- .../shapes/text/viewport_texts_html.cljs | 10 +- frontend/src/app/plugins/api.cljs | 16 +- frontend/src/app/plugins/reflow.cljs | 77 +++++ frontend/src/app/plugins/shape.cljs | 16 +- frontend/src/app/plugins/text.cljs | 8 +- frontend/src/app/plugins/utils.cljs | 8 - frontend/src/app/render_wasm/api/fonts.cljs | 141 ++++++-- .../data/workspace_reflow_test.cljs | 324 +++++++++++++++++- .../test/frontend_tests/main_errors_test.cljs | 280 +++++++-------- .../frontend_tests/plugins/comments_test.cljs | 88 ++--- .../plugins/context_shapes_test.cljs | 274 ++++++++++++++- .../test/frontend_tests/ui/routes_test.cljs | 124 ++++--- .../src/tests/wait-layout.test.ts | 110 +++++- plugins/apps/plugin-api-test-suite/src/ui.css | 33 +- plugins/apps/plugin-api-test-suite/src/ui.ts | 64 ++-- plugins/libs/plugin-types/index.d.ts | 16 +- 26 files changed, 1847 insertions(+), 652 deletions(-) create mode 100644 frontend/src/app/main/data/workspace/reflow/signals.cljs create mode 100644 frontend/src/app/plugins/reflow.cljs diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index d39d839ad7..fd2250c379 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -40,6 +40,7 @@ [app.main.data.workspace.groups :as dwg] [app.main.data.workspace.notifications :as-alias dwn] [app.main.data.workspace.pages :as-alias dwpg] + [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.selection :as dws] [app.main.data.workspace.shapes :as dwsh] [app.main.data.workspace.specialized-panel :as dwsp] @@ -1129,6 +1130,16 @@ (def valid-asset-types #{:colors :components :typographies}) +(defn- sync-file-pending-ids + [file-id changes] + ;; Track the file and every changed page object. + (into #{file-id} + (comp + (filter :page-id) + (keep :id) + (remove uuid/zero?)) + (:redo-changes changes))) + (defn set-updating-library [updating?] (ptk/reify ::set-updating-library @@ -1138,6 +1149,32 @@ (assoc state :updating-library true) (dissoc state :updating-library))))) +(defn- sync-file-frontend-events + [file-id changes updated-frames undo-group] + (rx/concat + (rx/of (set-updating-library false) + (ntf/hide {:tag :sync-dialog})) + (when (seq (:redo-changes changes)) + (rx/of (dch/commit-changes changes))) + (when-not (empty? updated-frames) + (let [frames-by-page (group-by :page-id updated-frames)] + (rx/merge + ;; Emit one layout/update event for each page. + (->> frames-by-page + (map (fn [[page-id frames]] + (ptk/data-event :layout/update + {:page-id page-id + :ids (map :id frames) + :undo-group undo-group}))) + (rx/from)) + (->> (rx/from updated-frames) + (rx/mapcat + (fn [shape] + (rx/of + (dwt/clear-thumbnail file-id (:page-id shape) (:id shape) "frame") + (when-not (= (:frame-id shape) uuid/zero) + (dwt/clear-thumbnail file-id (:page-id shape) (:frame-id shape) "frame"))))))))))) + (defn sync-file "Synchronize the given file from the given library. Walk through all shapes in all pages in the file that use some color, typography or @@ -1196,35 +1233,20 @@ updated-frames (->> changes :redo-changes (mapcat find-frames) - distinct)] + distinct) + + pending-ids (sync-file-pending-ids file-id changes) + + frontend-sync + (sync-file-frontend-events + file-id changes updated-frames undo-group)] (log/debug :msg "SYNC-FILE finished" :js/rchanges (log-changes (:redo-changes changes) ldata)) (rx/concat - (rx/of (set-updating-library false) - (ntf/hide {:tag :sync-dialog})) - (when (seq (:redo-changes changes)) - (rx/of (dch/commit-changes changes))) - (when-not (empty? updated-frames) - (let [frames-by-page (->> updated-frames - (group-by :page-id))] - (rx/merge - ;; Emit one layout/update event for each page - (rx/from - (map (fn [[page-id frames]] - (ptk/data-event :layout/update - {:page-id page-id - :ids (map :id frames) - :undo-group undo-group})) - frames-by-page)) - (->> (rx/from updated-frames) - (rx/mapcat - (fn [shape] - (rx/of - (dwt/clear-thumbnail file-id (:page-id shape) (:id shape) "frame") - (when-not (= (:frame-id shape) uuid/zero) - (dwt/clear-thumbnail file-id (:page-id shape) (:frame-id shape) "frame"))))))))) + ;; Keep the sync pending until its layout work starts. + (wrf/with-pending :sync-file pending-ids frontend-sync) (when (not= file-id library-id) ;; When we have just updated the library file, give some time for the @@ -1400,66 +1422,88 @@ (rx/buffer 2 1) (rx/map first)) - changes-s + ;; Barriers open before async inspection and close after detection. + pending-sync-barriers* (atom #{}) + + start-sync-barrier + (fn [{:keys [file-id save-undo?] :as event}] + (let [task (when (and save-undo? (uuid? file-id)) + (wrf/start! :sync-file [file-id]))] + (when task + (swap! pending-sync-barriers* conj task)) + [event task])) + + finish-sync-barrier! + (fn [task] + (when task + (wrf/finish! task) + (swap! pending-sync-barriers* disj task))) + + commits-s (->> stream (rx/filter dch/commit?) (rx/map deref) (rx/filter #(= :local (:source %))) + ;; Translation commits never propagate component changes. + (rx/filter (complement :translation?)) + ;; Keep waits pending while component changes are checked. + (rx/map start-sync-barrier) (rx/observe-on :async)) - check-changes + get-component-events (fn [[event old-data]] - (cond - (nil? old-data) - (rx/empty) + (let [{:keys [file-id changes save-undo? undo-group]} event + changed-components + (when (and old-data + (or (nil? file-id) (= file-id (:id old-data)))) + (into #{} + (mapcat (partial ch/components-changed old-data)) + changes))] + (cond + (empty? changed-components) + (rx/empty) - (:translation? event) - (rx/empty) + save-undo? + (do + (log/info :hint "detected component changes" + :ids (map str changed-components) + :undo-group undo-group) + (->> (rx/from changed-components) + (rx/map #(component-changed + % (:id old-data) undo-group)))) - :else - (let [{:keys [file-id changes save-undo? undo-group]} event + :else + ;; Undos only bump :modified-at. + (->> (rx/from changed-components) + (rx/map touch-component))))) - changed-components - (when (or (nil? file-id) (= file-id (:id old-data))) - (->> changes - (map (partial ch/components-changed old-data)) - (reduce into #{})))] - - (if (d/not-empty? changed-components) - (if save-undo? - (do (log/info :hint "detected component changes" - :ids (map str changed-components) - :undo-group undo-group) - (->> (rx/from changed-components) - (rx/map #(component-changed % (:id old-data) undo-group)))) - ;; save-undo? false (undos): just bump :modified-at - (->> (rx/from changed-components) - (rx/map touch-component))) - - (rx/empty))))) - - changes-s - (->> changes-s + component-events-s + (->> commits-s (rx/with-latest-from workspace-buffer-s) - (rx/mapcat check-changes) + (rx/mapcat + (fn [[[event task] old-data]] + (->> (get-component-events [event old-data]) + (rx/finalize #(finish-sync-barrier! task))))) + ;; Close barriers left behind when the page shuts down. + (rx/finalize #(wrf/finish-tasks! @pending-sync-barriers*)) (rx/share)) notifier-s - (->> changes-s + (->> component-events-s (rx/debounce 5000) (rx/tap #(log/trc :hint "buffer initialized")))] (when (or (contains? cf/flags :component-thumbnails) (features/active-feature? state "render-wasm/v1")) (->> (rx/merge - changes-s + component-events-s ;; WASM only: render the thumbnail on every component ;; change so single edits (fill, etc.) update instantly. ;; Non-WASM persists on every render, so it stays on the ;; debounced path below to avoid per-edit backend posts. (if (features/active-feature? state "render-wasm/v1") - (->> changes-s + (->> component-events-s (rx/filter (ptk/type? ::component-changed)) (rx/map deref) (rx/map render-component-thumbnail-event)) @@ -1467,7 +1511,7 @@ ;; Persist to the server in batches, 5s after the user ;; goes idle. - (->> changes-s + (->> component-events-s (rx/filter (ptk/type? ::component-changed)) (rx/map deref) (rx/buffer-until notifier-s) @@ -1476,7 +1520,7 @@ (update-component-thumbnail component-id file-id)))) ;; Undo/redo emit touch-component instead. - (->> changes-s + (->> component-events-s (rx/filter (ptk/type? ::touch-component)) (rx/map deref) (rx/map render-component-thumbnail-event))) @@ -1631,5 +1675,3 @@ (rx/mapcat (fn [_] (rp/cmd! :get-file-libraries {:file-id file-id}))) (rx/map (partial cleanup-unlinked-libraries file-id)))))) - - diff --git a/frontend/src/app/main/data/workspace/mcp.cljs b/frontend/src/app/main/data/workspace/mcp.cljs index fde7e22d6f..8931690c2b 100644 --- a/frontend/src/app/main/data/workspace/mcp.cljs +++ b/frontend/src/app/main/data/workspace/mcp.cljs @@ -14,6 +14,7 @@ [app.main.broadcast :as mbc] [app.main.data.plugins :as dp] [app.main.data.profile :as du] + [app.main.data.workspace :as-alias dw] [app.main.store :as st] [app.plugins.register :as preg] [app.util.timers :as ts] @@ -132,7 +133,7 @@ (assoc :host (str (u/join cf/public-uri "plugins/mcp/")))) stopper-s (rx/merge - (rx/filter (ptk/type? :app.main.data.workspace/finalize-workspace) stream) + (rx/filter (ptk/type? ::dw/finalize-workspace) stream) (rx/filter (ptk/type? ::stop-mcp-plugin) stream)) extension #js {:getToken (constantly token) @@ -202,7 +203,7 @@ ptk/WatchEvent (watch [_ state stream] (let [stopper-s (rx/merge - (rx/filter (ptk/type? :app.main.data.workspace/finalize-workspace) stream) + (rx/filter (ptk/type? ::dw/finalize-workspace) stream) (rx/filter (ptk/type? ::init) stream)) session-id (get state :session-id) diff --git a/frontend/src/app/main/data/workspace/reflow.cljs b/frontend/src/app/main/data/workspace/reflow.cljs index 3932aa7ca7..1fc4842672 100644 --- a/frontend/src/app/main/data/workspace/reflow.cljs +++ b/frontend/src/app/main/data/workspace/reflow.cljs @@ -5,11 +5,13 @@ ;; Copyright (c) KALEIDOS INC (ns app.main.data.workspace.reflow - "Tracks the shape ids that have layout/reflow work in flight, broken down by - the kind of work so we can tell which type of reflow is blocking each shape. + "Tracks the ids that have layout/reflow work in flight, broken down by the + kind of work so we can tell which type of reflow is blocking each id. - Pending work is stored as `{shape-id -> {kind -> #{task-id}}}`. Every producer - opens an exact task with `start!` and closes that same task with `finish!`. + Pending work is stored as `{id -> {kind -> #{task-id}}}`, where ids are page + object ids plus, for `:sync-file`, the id of the file being synced. Every + producer opens an exact task with `start!` and closes that same task with + `finish!`. Tasks belong to a workspace generation, so a delayed completion from a finalized workspace cannot drain work opened after the workspace reloads. @@ -21,8 +23,10 @@ :layout flex/grid layout reflow (shape-layout) :text-resize text geometry resize (wasm-text, texts) :text-measure DOM text measurement (texts) + :text-position DOM text fragment geometry (texts) :text-bridge change awaiting its pipeline (texts) - :font font change measurement (texts)" + :font font change measurement (texts) + :sync-file component/library propagation (libraries)" (:require [beicon.v2.core :as rx] [promesa.core :as p])) @@ -55,22 +59,35 @@ acc ids))) +;; Single-task operations are wrapped as batches before reaching the reducer. (defn- reducer - [acc {:keys [op task ids]}] + [acc {:keys [op tasks ids]}] (case op - :add (add-task acc task) - :remove (remove-task acc task) + :add (reduce add-task acc tasks) + :remove (reduce remove-task acc tasks) :cancel (apply dissoc acc ids) :reset {} acc)) -;; Behaviour subject holding `{shape-id -> {kind -> #{task-id}}}`. -;; It replays its current value synchronously to new subscribers, which gives -;; `wait-for-layout-update` a free fast-path when there is nothing pending. -(defonce ^:private pending-shapes - (let [sub (rx/behavior-subject {})] - (rx/sub! (->> reflow-input (rx/scan reducer {})) sub) - sub)) +;; Holds pending tasks and replays them to new waiters. +;; Reloads rebuild the scan with the latest reducer. +(def ^:private pending-shapes (rx/behavior-subject {})) + +(defonce ^:private pending-subscription (atom nil)) + +(defn- install-pending-subscription! + [] + ;; Settle the old scan before installing the new one. + (swap! workspace-generation inc) + (rx/push! reflow-input {:op :reset}) + (when-let [subscription @pending-subscription] + (rx/dispose! subscription)) + (reset! pending-subscription + (rx/sub! (->> reflow-input (rx/scan reducer {})) + pending-shapes)) + (rx/push! reflow-input {:op :reset})) + +(install-pending-subscription!) (defn task "Creates an opaque task token without opening it." @@ -80,24 +97,42 @@ :kind kind :ids (into #{} ids)}) +(defn- push-tasks! + [op tasks] + ;; Empty and stale tasks must not affect the active workspace. + (let [generation @workspace-generation + tasks (into [] (filter #(and (seq (:ids %)) + (= (:generation %) generation))) + tasks)] + (when (seq tasks) + (rx/push! reflow-input {:op op :tasks tasks})) + tasks)) + +(defn- start-tasks! + "Opens task tokens in one pending-map update." + [tasks] + (push-tasks! :add tasks)) + (defn start! "Opens and returns a task. The one-argument form opens a token created with `task`; the two-argument form creates and opens it in one step." ([task] - (when (and (seq (:ids task)) - (= (:generation task) @workspace-generation)) - (rx/push! reflow-input {:op :add :task task})) + (push-tasks! :add [task]) task) ([kind ids] (start! (task kind ids)))) +(defn finish-tasks! + "Closes task tokens from the active workspace generation in one update." + [tasks] + (push-tasks! :remove tasks) + nil) + (defn finish! "Closes `task` if it belongs to the active workspace generation. Repeated or stale completion is a no-op." - [{:keys [generation ids] :as task}] - (when (and (seq ids) - (= generation @workspace-generation)) - (rx/push! reflow-input {:op :remove :task task}))) + [task] + (finish-tasks! [task])) (defn reset-pending! "Starts a new workspace generation and forgets every task from the old one." @@ -136,59 +171,77 @@ (finish! task) (throw cause))))) -(defn pending-signal - "Emits once any of `kinds` is pending for any of `ids`, then completes. - Emits right away when that work is already in flight." - [ids kinds] - (letfn [(id-pending? [pending id] - (some (partial contains? (get pending id)) kinds)) +(defn bridge-pending + "Keeps each id pending until matching work starts." + [ids target-kinds bridge-kind] + (let [ids (into #{} ids)] + (if (empty? ids) + (rx/empty) + (rx/create + (fn [subs] + ;; Separate tasks let renderer work release each shape independently. + (let [tasks-by-id + (into {} (map (fn [id] [id (task bridge-kind [id])])) ids) - (any-pending? [pending] - (some (partial id-pending? pending) ids))] - (->> pending-shapes - (rx/filter any-pending?) - (rx/take 1)))) + remaining + (atom ids) -;; Ceiling for callers that pass no timeout, so a pipeline that never drains -;; its marks rejects the promise rather than leaving it unsettled. -(def ^:private default-timeout 30000) + release! + (fn [released] + (let [released (into #{} (filter @remaining) released)] + (when (seq released) + (finish-tasks! (map tasks-by-id released)) + (swap! remaining #(apply disj % released)) + (when (empty? @remaining) + (rx/end! subs))))) -(defn wait-for-layout-update - "Returns a JS Promise that resolves when every id in `shape-ids` has drained - from the pending map. A nil `shape-ids` waits for every pending shape; an - empty one has nothing to wait for and resolves right away. The promise is - rejected when `timeout` (ms) elapses first; a nil `timeout` uses - `default-timeout`. + matching-task-ids + (fn [tasks] + (into #{} + (comp + (filter #(contains? target-kinds (:kind %))) + (mapcat :ids) + (filter ids)) + tasks)) + + ;; Listen before opening bridges so synchronous work is not missed. + lifecycle-sub + (rx/sub! + reflow-input + (fn [{:keys [op tasks ids]}] + (case op + :add + (release! (matching-task-ids tasks)) + + :cancel + (release! ids) + + :reset + (release! @remaining) + + nil))) + + _ + (start-tasks! (vals tasks-by-id))] + (fn [] + (rx/dispose! lifecycle-sub) + (when (seq @remaining) + (finish-tasks! (map tasks-by-id @remaining)) + (reset! remaining #{}))))))))) + +(defn settled + "Observable that emits once every id in `ids` has drained from the pending + map, then completes. A nil `ids` waits for every pending id; an empty one has + nothing to wait for. Replays on subscribe, so an already drained map emits + immediately. Callers waiting on one shape pass its whole subtree: reflow work lands either on the shape (a board laying out its children) or on its descendants (a group whose texts are re-measured)." - ([timeout] - (wait-for-layout-update nil timeout)) - ([shape-ids timeout] - (js/Promise. - (fn [resolve reject] - (let [timeout (or timeout default-timeout) - - done? (if (some? shape-ids) - (fn [pending] (not-any? #(contains? pending %) shape-ids)) - empty?) - - settled (->> pending-shapes - (rx/filter done?) - (rx/map (constantly :ok))) - - ;; Race the settle signal against the deadline; the loser is - ;; unsubscribed. `settled` replays on subscribe, so an already - ;; drained map wins even against a 1ms deadline. - source (rx/race (->> (rx/of :timeout) - (rx/delay timeout)) - settled)] - (->> source - (rx/take 1) - (rx/subs! - (fn [value] - (if (= value :timeout) - (reject (js/Error. "waitForLayoutUpdate timeout")) - (resolve))) - reject))))))) + [ids] + (let [done? (if (some? ids) + (fn [pending] (not-any? #(contains? pending %) ids)) + empty?)] + (->> pending-shapes + (rx/filter done?) + (rx/take 1)))) diff --git a/frontend/src/app/main/data/workspace/reflow/signals.cljs b/frontend/src/app/main/data/workspace/reflow/signals.cljs new file mode 100644 index 0000000000..d2c145676e --- /dev/null +++ b/frontend/src/app/main/data/workspace/reflow/signals.cljs @@ -0,0 +1,141 @@ +;; 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 app.main.data.workspace.reflow.signals + "Decides which reflow signals a shape update raises: `:layout/update` for the + shapes whose layout attrs changed, `:text/reflow` for the texts the renderer + has to re-measure. + + Which text attrs matter depends on the renderer: the DOM one measures every + changed text, so its own geometry counts as a change; wasm only resizes + auto-sized texts from their content." + (:require + [app.common.data :as d] + [app.common.data.macros :as dm] + [app.common.files.changes-builder :as pcb] + [app.common.files.helpers :as cfh] + [app.common.math :as mth] + [app.main.features :as features])) + +;; If anything a translation can mutate is added here, drop the +;; `(when-not translation? …)` guard in `update-shapes`. +(def ^:private update-layout-attr? #{:hidden}) + +;; Text attrs that can start async renderer work. +(def ^:private text-reflow-attr? + #{:content :grow-type :x :y :width :height}) + +(def ^:private wasm-text-reflow-attr? + #{:content :grow-type}) + +(def ^:private dom-text-geometry-reflow-attr? + #{:x :y :width :height}) + +(defn- renderer-text-reflow-attr? + [state] + (if (features/active-feature? state "render-wasm/v1") + wasm-text-reflow-attr? + text-reflow-attr?)) + +(defn- reflow-attr? + [state attr] + (or (update-layout-attr? attr) + ((renderer-text-reflow-attr? state) attr))) + +;; Caller metadata can rule out reflow before objects are compared. +(defn- reflow-candidate? + [attr? {:keys [attrs translation? update-layout?] + :or {update-layout? true}}] + (and update-layout? + (not translation?) + (or (nil? attrs) + (some attr? attrs)))) + +(defn- text-reflow-changed? + [state shape changed-shape changed] + ;; Match the DOM renderer's geometry checks. + (let [wasm? (features/active-feature? state "render-wasm/v1") + reflow-attr? (renderer-text-reflow-attr? state)] + (some + (fn [attr] + (and (reflow-attr? attr) + (or wasm? + (not (dom-text-geometry-reflow-attr? attr)) + (not (mth/close? (get shape attr) + (get changed-shape attr)))))) + changed))) + +(defn- async-text-reflow? + "Whether `shape` enters an asynchronous text geometry pipeline. The HTML + renderer measures every changed text; WASM only resizes auto-sized texts. + A grow-type transition is included because `shape` is the value before the + update and may still be fixed." + [state shape changed] + (and (cfh/text-shape? shape) + (or (not (features/active-feature? state "render-wasm/v1")) + (not= :fixed (:grow-type shape)) + (contains? changed :grow-type)))) + +(defn- get-reflow-changes + [state objects changed-objects ids {:keys [attrs] :as props}] + ;; Reuse built objects so update functions only run once. + (let [reflow-attr? (partial reflow-attr? state)] + (when (reflow-candidate? reflow-attr? props) + (into [] + (comp + (map (d/getf objects)) + (keep (fn [shape] + (let [changed-shape (get changed-objects (:id shape)) + changed (pcb/changed-attrs + shape objects (constantly changed-shape) + {:attrs attrs})] + (when (some reflow-attr? changed) + [shape changed-shape changed]))))) + ids)))) + +(defn- get-layout-reflow-ids + [reflow-changes] + (->> reflow-changes + (into [] (comp (filter (fn [[_ _ changed]] (some update-layout-attr? changed))) + (map (comp :id first)))) + (not-empty))) + +(defn- get-text-reflow-ids + [state page-id reflow-changes] + ;; Track measurable texts on the active page. + (when (= page-id (get state :current-page-id)) + (let [edition (dm/get-in state [:workspace-local :edition])] + (->> reflow-changes + (into [] (comp (filter (fn [[shape changed-shape changed]] + (and (async-text-reflow? state shape changed) + (text-reflow-changed? + state shape changed-shape changed)))) + (map (comp :id first)) + (remove #(= % edition)))) + (not-empty))))) + +(defn reflow-ids + "Ids a shape update has to signal: `:layout-ids` for `:layout/update`, + `:text-ids` for `:text/reflow`. Both are nil when nothing changed. + + Both sets come from one comparison pass, so `update-fn` and the attribute + diff only run once per shape." + [state page-id objects changed-objects ids props] + (let [reflow-changes (get-reflow-changes state objects changed-objects ids props)] + {:layout-ids (get-layout-reflow-ids reflow-changes) + :text-ids (get-text-reflow-ids state page-id reflow-changes)})) + +(defn text-reflow-candidate? + "Whether `props` can start renderer text work, judged from the caller metadata + alone. Cheap pre-filter for callers that buffer updates before they have + objects to compare." + [state props] + (reflow-candidate? (renderer-text-reflow-attr? state) props)) + +(defn new-text-reflow? + "Whether a newly added `shape` enters an asynchronous text geometry pipeline." + [state shape] + (async-text-reflow? state shape nil)) diff --git a/frontend/src/app/main/data/workspace/selection.cljs b/frontend/src/app/main/data/workspace/selection.cljs index 36039f230b..838d6cc02e 100644 --- a/frontend/src/app/main/data/workspace/selection.cljs +++ b/frontend/src/app/main/data/workspace/selection.cljs @@ -29,6 +29,7 @@ [app.main.data.workspace.undo :as dwu] [app.main.data.workspace.viewport-wasm :as dwvw] [app.main.data.workspace.zoom :as dwz] + [app.main.features :as features] [app.main.refs :as refs] [app.main.router :as rt] [app.main.streams :as ms] @@ -452,6 +453,16 @@ (gpt/subtract new-pos pt-obj))))) +(defn- get-new-dom-text-ids + [state changes] + (when-not (features/active-feature? state "render-wasm/v1") + (->> (:redo-changes changes) + (keep (fn [{:keys [type obj]}] + (when (and (= type :add-obj) + (cfh/text-shape? obj)) + (:id obj)))) + (not-empty)))) + (defn duplicate-shapes [ids & {:keys [move-delta? alt-duplication? change-selection? return-ref] :or {move-delta? false alt-duplication? false change-selection? true return-ref nil}}] @@ -493,6 +504,9 @@ (map #(get-in % [:obj :id])) (into (d/ordered-set))) + new-dom-text-ids + (get-new-dom-text-ids state changes) + id-duplicated (first new-ids) frames (into #{} @@ -531,6 +545,11 @@ ;; Warning: This order is important for the focus mode. (->> (rx/of (dwu/start-undo-transaction undo-id) + ;; Track cloned texts before they mount. + (when new-dom-text-ids + (ptk/data-event :text/reflow + {:ids new-dom-text-ids + :page-id (:id page)})) (dch/commit-changes changes) (when change-selection? (select-shapes new-ids)) diff --git a/frontend/src/app/main/data/workspace/shape_layout.cljs b/frontend/src/app/main/data/workspace/shape_layout.cljs index fade07bd2d..d4c09ec75e 100644 --- a/frontend/src/app/main/data/workspace/shape_layout.cljs +++ b/frontend/src/app/main/data/workspace/shape_layout.cljs @@ -23,6 +23,7 @@ [app.main.data.changes :as dch] [app.main.data.event :as ev] [app.main.data.helpers :as dsh] + [app.main.data.workspace :as-alias dw] [app.main.data.workspace.colors :as cl] [app.main.data.workspace.grid-layout.editor :as dwge] [app.main.data.workspace.modifiers :as dwm] @@ -131,14 +132,14 @@ (->> stream (rx/filter (ptk/type? ::dwsh/update-shapes-buffer-commit)) (rx/take 1) - (rx/take-until (rx/filter (ptk/type? :app.main.data.workspace/finalize) stream)) + (rx/take-until (rx/filter (ptk/type? ::dw/finalize-workspace) stream)) ;; No events are derived from this (rx/ignore)) (rx/empty))] (cond->> (rx/concat update-positions-stream drain-stream) (d/not-empty? reflow-tasks) - (rx/finalize #(run! wrf/finish! reflow-tasks))))))) + (rx/finalize #(wrf/finish-tasks! reflow-tasks))))))) (defn- without-root-board [ids] diff --git a/frontend/src/app/main/data/workspace/shapes.cljs b/frontend/src/app/main/data/workspace/shapes.cljs index 0b3cfb3f94..3530e35f3b 100644 --- a/frontend/src/app/main/data/workspace/shapes.cljs +++ b/frontend/src/app/main/data/workspace/shapes.cljs @@ -24,34 +24,12 @@ [app.main.data.workspace.collapse :as dwco] [app.main.data.workspace.edition :as dwe] [app.main.data.workspace.reflow :as wrf] + [app.main.data.workspace.reflow.signals :as wrfs] [app.main.data.workspace.selection :as dws] [app.main.data.workspace.undo :as dwu] - [app.main.features :as features] [beicon.v2.core :as rx] [potok.v2.core :as ptk])) -;; If anything a translation can mutate is added here, drop the -;; `(when-not translation? …)` guard in `update-shapes` below. -(def ^:private update-layout-attr? #{:hidden}) - -;; Text attrs whose change makes the DOM text pipeline re-measure the shape. -(def ^:private text-reflow-attr? #{:content :grow-type}) - -(defn- reflow-attr? - [attr] - (or (update-layout-attr? attr) (text-reflow-attr? attr))) - -(defn- async-text-reflow? - "Whether `shape` enters an asynchronous text geometry pipeline. The HTML - renderer measures every changed text; WASM only resizes auto-sized texts. - A grow-type transition is included because `shape` is the value before the - update and may still be fixed." - [state shape changed] - (and (cfh/text-shape? shape) - (or (not (features/active-feature? state "render-wasm/v1")) - (not= :fixed (:grow-type shape)) - (contains? changed :grow-type)))) - (defn- add-undo-group [changes state] (let [undo (:workspace-undo state) @@ -82,15 +60,35 @@ (update [_ state] (assoc state ::update-shapes-buffer false)))) +(defn- get-buffered-text-reflow-event + [state page-id ids] + (when (= page-id (get state :current-page-id)) + ;; Analyze accumulated objects through the same path as immediate updates. + (let [objects (dsh/lookup-page-objects state page-id) + changed-objects (-> (get-in state [::update-shapes-buffer-changes page-id]) + (pcb/lookup-objects)) + {:keys [text-ids]} + (wrfs/reflow-ids state page-id objects changed-objects ids nil)] + (when text-ids + (ptk/data-event :text/reflow {:ids text-ids :page-id page-id}))))) + (defn update-shapes-buffer-commit [] (ptk/reify ::update-shapes-buffer-commit ptk/WatchEvent (watch [_ state _] - (->> (get state ::update-shapes-buffer-changes) - (vals) - (map dch/commit-changes) - (rx/from))))) + (let [text-reflow-events + (->> (get state ::update-shapes-buffer-text-candidates) + (keep (fn [[page-id ids]] + (get-buffered-text-reflow-event state page-id ids)))) + + commits + (->> (get state ::update-shapes-buffer-changes) + (vals) + (map dch/commit-changes))] + ;; Open bridges before commits start rendering. + (rx/concat (rx/from text-reflow-events) + (rx/from commits)))))) ;; Looks for the objects data in the state, if there is an "in progress" ;; update-shapes-buffer will return the objeccts inside the current changes @@ -111,7 +109,8 @@ (update-shapes-buffer ids update-fn nil)) ([ids update-fn {:keys [reg-objects? save-undo? stack-undo? attrs ignore-tree page-id - ignore-touched undo-group with-objects? changed-sub-attr translation?] + ignore-touched undo-group with-objects? changed-sub-attr + translation?] :or {reg-objects? false save-undo? true stack-undo? false @@ -126,9 +125,14 @@ (assoc state ::update-shapes-buffer-event cur-event) (let [page-id (or page-id (get state :current-page-id)) - objects (dsh/lookup-page-objects state page-id)] - (-> state + objects (lookup-changed-objects state page-id) + text-ids + (into #{} + (filter #(cfh/text-shape? objects %)) + ids) + state (update-in + state [::update-shapes-buffer-changes page-id] (fn [changes] (-> (or changes @@ -148,7 +152,15 @@ :ignore-touched ignore-touched :with-objects? with-objects?}) (cond-> reg-objects? (pcb/resize-parents ids)) - (pcb/set-translation? translation?)))))))) + (pcb/set-translation? translation?))))] + ;; Check buffered text candidates when the buffer is committed. + (if (or (empty? text-ids) + (not (wrfs/text-reflow-candidate? state props))) + state + (update-in state + [::update-shapes-buffer-text-candidates page-id] + (fnil into #{}) + text-ids))))) ptk/WatchEvent (watch [_ state stream] @@ -165,6 +177,7 @@ (rx/of #(dissoc % ::update-shapes-buffer-changes + ::update-shapes-buffer-text-candidates ::update-shapes-buffer-event)))) (rx/empty))))))) @@ -174,14 +187,12 @@ ([ids update-fn {:as props :keys [reg-objects? save-undo? stack-undo? attrs ignore-tree page-id - ignore-touched undo-group with-objects? changed-sub-attr translation? - update-layout?] + ignore-touched undo-group with-objects? changed-sub-attr translation?] :or {reg-objects? false save-undo? true stack-undo? false ignore-touched false - with-objects? false - update-layout? true}}] + with-objects? false}}] (assert (every? uuid? ids) "expect a coll of uuid for `ids`") (assert (fn? update-fn) "the `update-fn` should be a valid function") @@ -197,49 +208,6 @@ objects (dsh/lookup-page-objects state page-id) ids (into [] (filter some?) ids) - ;; Pairs of [shape changed-attrs] for the shapes whose change - ;; matters to a reflow, feeding both id sets below. - xf-reflow - (comp - (map (d/getf objects)) - (keep (fn [shape] - (let [changed (pcb/changed-attrs shape objects update-fn - {:attrs attrs :with-objects? with-objects?})] - (when (some reflow-attr? changed) - [shape changed]))))) - - ;; `changed-attrs` runs `update-fn` in full for every shape, which - ;; can be expensive (e.g. `update-bool-shape` recalculates the whole - ;; boolean path in WASM). Skip the pass entirely when we can prove it - ;; cannot match: when the caller declares `attrs`, `changed-attrs` - ;; filters its result to that set, so if no reflow attr is present - ;; the check is always empty. - reflow-changes - (when-not (or translation? - (not update-layout?) - (and (some? attrs) - (not (some reflow-attr? attrs)))) - (into [] xf-reflow ids)) - - update-layout-ids - (->> reflow-changes - (into [] (comp (filter (fn [[_ changed]] (some update-layout-attr? changed))) - (map (comp :id first)))) - (not-empty)) - - ;; Text shapes the DOM pipeline has to re-measure, narrowed to what - ;; it actually measures: the active page, never the edited shape. - text-reflow-ids - (when (= page-id (get state :current-page-id)) - (let [edition (dm/get-in state [:workspace-local :edition])] - (->> reflow-changes - (into [] (comp (filter (fn [[shape changed]] - (and (async-text-reflow? state shape changed) - (some text-reflow-attr? changed)))) - (map (comp :id first)) - (remove #(= % edition)))) - (not-empty)))) - changes (-> (pcb/empty-changes it page-id) (pcb/set-save-undo? save-undo?) @@ -257,6 +225,12 @@ (pcb/set-undo-group undo-group)) (pcb/set-translation? translation?)) + changed-objects + (pcb/lookup-objects changes) + + {:keys [layout-ids text-ids]} + (wrfs/reflow-ids state page-id objects changed-objects ids props) + changes (add-undo-group changes state)] @@ -264,8 +238,8 @@ ;; Announces the texts still to be re-measured, so a reflow wait ;; covers the render that measures them. Goes before the commit, ;; which is what triggers that render. - (if text-reflow-ids - (rx/of (ptk/data-event :text/reflow {:ids text-reflow-ids :page-id page-id})) + (if text-ids + (rx/of (ptk/data-event :text/reflow {:ids text-ids :page-id page-id})) (rx/empty)) (if (seq (:redo-changes changes)) @@ -274,8 +248,8 @@ (rx/empty)) ;; Update layouts for properties marked - (if update-layout-ids - (rx/of (ptk/data-event :layout/update {:ids update-layout-ids})) + (if layout-ids + (rx/of (ptk/data-event :layout/update {:ids layout-ids})) (rx/empty))))))))) (defn add-shape @@ -321,7 +295,7 @@ (rx/of (dwu/start-undo-transaction undo-id) ;; A new text has no geometry until the pipeline measures it, ;; so it raises the same signal an edit does. - (when (async-text-reflow? state shape nil) + (when (wrfs/new-text-reflow? state shape) (ptk/data-event :text/reflow {:ids [(:id shape)] :page-id page-id})) (dch/commit-changes changes) (when-not no-update-layout? diff --git a/frontend/src/app/main/data/workspace/texts.cljs b/frontend/src/app/main/data/workspace/texts.cljs index fb33068adc..3db75ccb8e 100644 --- a/frontend/src/app/main/data/workspace/texts.cljs +++ b/frontend/src/app/main/data/workspace/texts.cljs @@ -24,9 +24,11 @@ [app.main.data.changes :as dch] [app.main.data.event :as ev] [app.main.data.helpers :as dsh] + [app.main.data.workspace :as-alias dw] [app.main.data.workspace.common :as dwc] [app.main.data.workspace.libraries :as dwl] [app.main.data.workspace.modifiers :as dwm] + [app.main.data.workspace.pages :as-alias dwpg] [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.selection :as dws] [app.main.data.workspace.shapes :as dwsh] @@ -66,15 +68,19 @@ "Marks `ids` pending until the text pipeline marks its own work: `:text-measure` in the DOM renderer, `:text-resize` in wasm. Emits nothing." [ids] - (->> (rx/from ids) - ;; Each id owns its bridge. Starting work for one text must not release - ;; siblings that the renderer has not picked up yet. - (rx/mapcat - (fn [id] - (->> (wrf/pending-signal [id] #{:text-measure :text-resize}) - (rx/ignore) - (wrf/with-pending :text-bridge [id])))) - (rx/ignore))) + (wrf/bridge-pending ids #{:text-measure :text-resize} :text-bridge)) + +(defn- page-finalize? + [event] + (= ::dwpg/finalize-page (ptk/type event))) + +(defn- text-work-stopper + [stream] + (rx/filter + (fn [event] + (or (= ::dw/finalize-workspace (ptk/type event)) + (page-finalize? event))) + stream)) (defn initialize-text-reflow "Tracks the texts the DOM pipeline still has to re-measure, so a reflow wait @@ -83,11 +89,15 @@ (ptk/reify ::initialize-text-reflow ptk/WatchEvent (watch [_ _ stream] - (let [stopper (rx/filter (ptk/type? ::finalize-text-reflow) stream)] + (let [stopper (rx/filter (ptk/type? ::finalize-text-reflow) stream) + page-stopper (rx/filter page-finalize? stream)] (->> stream (rx/filter (ptk/type? :text/reflow)) (rx/map deref) - (rx/mapcat (fn [{:keys [ids]}] (bridge-to-measurement ids))) + (rx/merge-map + (fn [{:keys [ids]}] + (->> (bridge-to-measurement ids) + (rx/take-until page-stopper)))) (rx/take-until stopper)))))) (defn finalize-text-reflow @@ -110,28 +120,51 @@ :else []))) +(defn- await-font-faces + "Waits for missing WASM faces, then resizes the affected texts." + [stream face-keys ids] + (let [resize-stream (->> (rx/from ids) (rx/map dwwt/resize-wasm-text))] + (if (empty? face-keys) + resize-stream + (->> (rx/merge wasm.fonts/font-stored-stream + wasm.fonts/font-storage-failed-stream) + (rx/filter face-keys) + (rx/scan disj face-keys) + (rx/filter empty?) + (rx/take 1) + (rx/take-until (text-work-stopper stream)) + (rx/observe-on :async) + (rx/mapcat (constantly resize-stream)) + (wrf/with-pending :font ids))))) + +(defn- pending-font-faces + [ids] + (let [objects (dsh/lookup-page-objects @st/state)] + (into #{} + (comp + (map #(get objects %)) + (keep :content) + (mapcat wasm.fonts/get-content-fonts) + (map wasm.fonts/make-font-data) + (remove wasm.fonts/font-ready?) + (map wasm.fonts/font-data-key)) + ids))) + (defn- await-font-resize - "Marks `ids` as pending font work and dispatches their wasm resize once wasm - can measure with `font-id`, draining the marks afterwards. The fetch of that - font is started by the wasm shape sync of the content change these shapes - receive, so measuring before it lands would use the fallback font." - [stream font-id ids] + "Waits for missing font faces, then resizes `ids`." + [stream ids] (if (empty? ids) (rx/empty) - (let [stopper (rx/filter (ptk/type? :app.main.data.workspace/finalize) stream)] - (->> wasm.fonts/font-stored-stream - (rx/filter #(= % font-id)) - (rx/take 1) - (rx/take-until stopper) - (rx/observe-on :async) - (rx/mapcat (fn [_] (rx/from (mapv dwwt/resize-wasm-text ids)))) - (wrf/with-pending :font ids))))) + (->> (rx/of ::await-fonts) + (rx/mapcat + (fn [_] + (await-font-faces stream (pending-font-faces ids) ids)))))) (defn- await-html-font "Keeps legacy DOM text pending while its new font is loading. The DOM measurement also awaits this promise, so the font task bridges the state update to the renderer commit without relying on a fixed settle delay." - [font-id font-variant-id ids] + [stream font-id font-variant-id ids] (if (or (nil? font-id) (empty? ids)) (rx/empty) (->> (rx/of ::load-font) @@ -140,6 +173,7 @@ ;; gap before the task is visible to waiters. (rx/mapcat (fn [_] (rx/from (fonts/ensure-loaded! font-id font-variant-id)))) + (rx/take-until (text-work-stopper stream)) (rx/ignore) (wrf/with-pending :font ids)))) @@ -525,7 +559,7 @@ [id start end attrs] (ptk/reify ::update-text-range ptk/WatchEvent - (watch [_ state _] + (watch [_ state stream] (let [objects (dsh/lookup-page-objects state) shape (get objects id) @@ -547,7 +581,7 @@ (rx/map dwwt/resize-wasm-text-debounce)) (contains? attrs :font-id) - (await-html-font (:font-id attrs) (:font-variant-id attrs) text-ids) + (await-html-font stream (:font-id attrs) (:font-variant-id attrs) text-ids) :else (rx/empty))))))) @@ -798,7 +832,7 @@ (watch [_ state stream] (wrf/start! reflow-task) (if (= (::resize-text-debounce-event state) cur-event) - (let [stopper (->> stream (rx/filter (ptk/type? :app.main.data.workspace/finalize)))] + (let [stopper (->> stream (rx/filter (ptk/type? ::dw/finalize-workspace)))] (rx/concat (rx/merge (->> stream @@ -809,7 +843,7 @@ (rx/take-until stopper)) (rx/of (resize-text id new-width new-height))) (rx/of (fn [state] - (run! wrf/finish! (::resize-text-reflow-tasks state)) + (wrf/finish-tasks! (::resize-text-reflow-tasks state)) (dissoc state ::resize-text-debounce-props ::resize-text-reflow-tasks @@ -878,7 +912,7 @@ ptk/WatchEvent (watch [_ state stream] (if (= (::update-text-modifier-debounce-event state) cur-event) - (let [stopper (->> stream (rx/filter (ptk/type? :app.main.data.workspace/finalize)))] + (let [stopper (->> stream (rx/filter (ptk/type? ::dw/finalize-workspace)))] (rx/concat (rx/merge (->> stream @@ -926,40 +960,49 @@ ptk/WatchEvent (watch [_ state _] (let [position-data (::update-position-data state)] - (rx/concat - (rx/of (dwsh/update-shapes - (keys position-data) - (fn [shape] - (-> shape - (assoc :position-data (get position-data (:id shape))))) - {:stack-undo? true :reg-objects? false})) - (rx/of (fn [state] - (dissoc state ::update-position-data-debounce ::update-position-data)))))))) + (rx/of (dwsh/update-shapes + (keys position-data) + (fn [shape] + (-> shape + (assoc :position-data (get position-data (:id shape))))) + {:stack-undo? true :reg-objects? false})))))) (defn update-position-data [id position-data] - (let [cur-event (js/Symbol)] + (let [cur-event (js/Symbol) + reflow-task (wrf/task :text-position [id])] (ptk/reify ::update-position-data ptk/UpdateEvent (update [_ state] (let [state (assoc-in state [:workspace-text-modifier id :position-data] position-data)] - (if (nil? (::update-position-data-debounce state)) - (assoc state ::update-position-data-debounce cur-event) - (assoc-in state [::update-position-data id] position-data)))) + (-> state + (update ::update-position-data-reflow-tasks (fnil conj []) reflow-task) + (cond-> (nil? (::update-position-data-debounce state)) + (assoc ::update-position-data-debounce cur-event)) + (cond-> (some? (::update-position-data-debounce state)) + (assoc-in [::update-position-data id] position-data))))) ptk/WatchEvent (watch [_ state stream] + (wrf/start! reflow-task) (if (= (::update-position-data-debounce state) cur-event) - (let [stopper (->> stream (rx/filter (ptk/type? :app.main.data.workspace/finalize)))] - (rx/merge - (->> stream - (rx/filter (ptk/type? ::update-position-data)) - (rx/debounce 50) - (rx/take 1) - (rx/map #(commit-position-data)) - (rx/take-until stopper)) - (rx/of (update-position-data id position-data)))) + (let [stopper (text-work-stopper stream)] + (rx/concat + (rx/merge + (->> stream + (rx/filter (ptk/type? ::update-position-data)) + (rx/debounce 50) + (rx/take 1) + (rx/map #(commit-position-data)) + (rx/take-until stopper)) + (rx/of (update-position-data id position-data))) + (rx/of (fn [state] + (wrf/finish-tasks! (::update-position-data-reflow-tasks state)) + (dissoc state + ::update-position-data-debounce + ::update-position-data + ::update-position-data-reflow-tasks))))) (rx/empty)))))) (defn update-attrs @@ -1010,7 +1053,7 @@ (let [auto-ids (into [] (remove #(= :fixed (:grow-type (get objects %)))) text-ids)] (if (contains? attrs :font-id) ;; The geometry depends on the font, so wait until wasm has it. - (await-font-resize stream (:font-id attrs) auto-ids) + (await-font-resize stream auto-ids) ;; No font change: measurable right away. (->> (rx/from auto-ids) (rx/map dwwt/resize-wasm-text))))) @@ -1019,6 +1062,7 @@ ;; but font loading starts before that render commits. (if (contains? attrs :font-id) (await-html-font + stream (:font-id attrs) (:font-variant-id attrs) text-ids) diff --git a/frontend/src/app/main/data/workspace/wasm_text.cljs b/frontend/src/app/main/data/workspace/wasm_text.cljs index eba1fdb8f6..bb98793dc3 100644 --- a/frontend/src/app/main/data/workspace/wasm_text.cljs +++ b/frontend/src/app/main/data/workspace/wasm_text.cljs @@ -16,6 +16,7 @@ [app.common.geom.point :as gpt] [app.common.types.modifiers :as ctm] [app.main.data.helpers :as dsh] + [app.main.data.workspace :as-alias dw] [app.main.data.workspace.modifiers :as dwm] [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.shapes :as dwsh] @@ -159,7 +160,7 @@ (watch [_ state stream] (wrf/start! reflow-task) (if (= (::resize-wasm-text-debounce-event state) cur-event) - (let [stopper (->> stream (rx/filter (ptk/type? :app.main.data.workspace/finalize)))] + (let [stopper (->> stream (rx/filter (ptk/type? ::dw/finalize-workspace)))] (rx/concat (rx/merge (->> stream @@ -179,7 +180,7 @@ ;; pending until the resize is applied. All exact tasks in the ;; batch are retained in state and finished by the cleanup. (rx/of (fn [state] - (run! wrf/finish! (::resize-wasm-text-reflow-tasks state)) + (wrf/finish-tasks! (::resize-wasm-text-reflow-tasks state)) (dissoc state ::resize-wasm-text-debounce-ids ::resize-wasm-text-reflow-tasks @@ -198,15 +199,15 @@ content (dm/get-in objects [id :content]) fonts (wasm.fonts/get-content-fonts content) - fonts-loaded? + fonts-ready? (->> fonts (every? (fn [font] (let [font-data (wasm.fonts/make-font-data font)] - (wasm.fonts/font-stored? font-data (:emoji? font-data)))))) + (wasm.fonts/font-ready? font-data))))) resize-wasm-stream - (if fonts-loaded? + (if fonts-ready? (let [pass-opts (when (or (some? undo-group) (some? undo-id)) (cond-> {} (some? undo-group) (assoc :undo-group undo-group) @@ -232,15 +233,32 @@ (watch [_ state stream] (let [resize-stream (->> (rx/from ids) - (rx/map #(resize-wasm-text-debounce % opts)))] + (rx/map #(resize-wasm-text-debounce % opts))) + + buffer-finished-stream + (->> (rx/merge + (->> stream + (rx/filter (ptk/type? ::dwsh/update-shapes-buffer-commit)) + (rx/map (constantly :commit))) + ;; Let a buffered commit beat the stop signal. + (->> stream + (rx/filter (ptk/type? ::dwsh/update-shapes-buffer-stop)) + (rx/observe-on :async) + (rx/map (constantly :stop))) + (->> stream + (rx/filter (ptk/type? ::dw/finalize-workspace)) + (rx/map (constantly :finalize)))) + (rx/take 1))] (if (::dwsh/update-shapes-buffer state) ;; If we're in the middle of a token propagation we wait until is finished to ;; recalculate the text sizes. The shapes stay pending for that whole wait, ;; since the per-shape debounce only marks them once dispatched. (wrf/with-pending :text-resize ids - (->> stream - (rx/filter (ptk/type? ::dwsh/update-shapes-buffer-commit)) - (rx/take 1) - (rx/mapcat (constantly resize-stream)))) + (->> buffer-finished-stream + (rx/mapcat + (fn [reason] + (if (= reason :finalize) + (rx/empty) + resize-stream))))) resize-stream)))))) diff --git a/frontend/src/app/main/fonts.cljs b/frontend/src/app/main/fonts.cljs index 677f8aa1fb..15fa05534b 100644 --- a/frontend/src/app/main/fonts.cljs +++ b/frontend/src/app/main/fonts.cljs @@ -18,6 +18,7 @@ [app.util.globals :as globals] [app.util.http :as http] [app.util.object :as obj] + [app.util.timers :as tm] [beicon.v2.core :as rx] [cuerdas.core :as str] [okulary.core :as l] @@ -243,8 +244,10 @@ (defmulti ^:private load-font :backend) (defmethod load-font :default - [{:keys [backend] :as font}] - (log/wrn :msg "no implementation found for" :backend backend)) + [{:keys [backend ::on-failed] :as font}] + (log/wrn :msg "no implementation found for" :backend backend) + (when (fn? on-failed) + (on-failed (ex-info "unsupported font backend" {:backend backend})))) (defmethod load-font :builtin [{:keys [id ::on-loaded] :as font}] @@ -269,23 +272,30 @@ (let [base (u/join cf/public-uri "internal/gfonts/font")] (str/replace css "https://fonts.gstatic.com/s" (dm/str base)))) -(defn- fetch-gfont-css +(defn- request-gfont-css [url] (->> (http/send! {:method :get :uri url :mode :cors :response-type :text}) - (rx/map :body) - (rx/catch (fn [err] - (log/wrn :hint "cannot find the font" :cause err) + (rx/map :body))) + +(defn- fetch-gfont-css + [url] + (->> (request-gfont-css url) + (rx/catch (fn [cause] + ;; Keep CSS streams alive when a font cannot load. + (log/wrn :hint "cannot find the font" :cause cause) (rx/empty))))) (defmethod load-font :google - [{:keys [id ::on-loaded] :as font}] + [{:keys [id ::on-loaded ::on-failed] :as font}] (when (globals/browser?) (log/dbg :hint "load-font" :font-id id :backend "google") (let [url (generate-gfonts-url font)] - (->> (fetch-gfont-css url) + ;; Keep raw errors so the loader can use its fallback. + (->> (request-gfont-css url) (rx/map process-gfont-css) (rx/tap #(on-loaded id)) - (rx/subs! (partial add-font-css! id))) + (rx/subs! (partial add-font-css! id) + #(when (fn? on-failed) (on-failed %)))) nil))) ;; --- LOADER: CUSTOM @@ -358,15 +368,30 @@ ;; First caller, we create the promise and then wait :else - (let [on-load (fn [resolve] - (swap! loaded conj font-id) - (swap! loading dissoc font-id) - (resolve font-id)) + (let [settle! (fn [resolve loaded?] + ;; Defer cleanup until a synchronous load is cached. + (tm/schedule + #(do + (when loaded? + (swap! loaded conj font-id)) + (swap! loading dissoc font-id) + (resolve font-id)))) + + on-load (fn [resolve] + (settle! resolve true)) + + on-failed + (fn [resolve cause] + (log/wrn :hint "font load failed; using fallback" + :font-id font-id + :cause cause) + (settle! resolve false)) load-p (-> (p/create (fn [resolve _] (-> font (assoc ::on-loaded (partial on-load resolve)) + (assoc ::on-failed (partial on-failed resolve)) (load-font)))) ;; We need to wait for the font to be loaded (p/then (partial p/delay 120)))] diff --git a/frontend/src/app/main/ui/workspace/shapes/text/viewport_texts_html.cljs b/frontend/src/app/main/ui/workspace/shapes/text/viewport_texts_html.cljs index 77d659c17b..634392e97a 100644 --- a/frontend/src/app/main/ui/workspace/shapes/text/viewport_texts_html.cljs +++ b/frontend/src/app/main/ui/workspace/shapes/text/viewport_texts_html.cljs @@ -12,6 +12,7 @@ [app.common.geom.point :as gpt] [app.common.geom.shapes :as gsh] [app.common.geom.shapes.text :as gsht] + [app.common.logging :as log] [app.common.math :as mth] [app.common.types.modifiers :as ctm] [app.common.types.text :as txt] @@ -96,9 +97,12 @@ (st/emit! (dwt/resize-text id width height))))) (st/emit! (dwt/clean-text-modifier id)))) - ;; Swallowed so a text whose position data cannot be computed still - ;; settles and still reports its measurement as finished. - (p/catch (fn [_] nil)))) + ;; Always clear the task and log measurement errors. + (p/catch (fn [cause] + (log/error :hint "Could not measure text shape" + :shape-id id + :cause cause) + nil)))) (defn- update-text-modifier [{:keys [grow-type id] :as shape} node] diff --git a/frontend/src/app/plugins/api.cljs b/frontend/src/app/plugins/api.cljs index bea36ad027..6582b76e62 100644 --- a/frontend/src/app/plugins/api.cljs +++ b/frontend/src/app/plugins/api.cljs @@ -28,7 +28,6 @@ [app.main.data.workspace.groups :as dwg] [app.main.data.workspace.media :as dwm] [app.main.data.workspace.pages :as dwpg] - [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.selection :as dws] [app.main.data.workspace.variants :as dwv] [app.main.data.workspace.wasm-text :as dwwt] @@ -47,6 +46,7 @@ [app.plugins.local-storage :as local-storage] [app.plugins.page :as page] [app.plugins.parser :as parser] + [app.plugins.reflow :as wrfp] [app.plugins.shape :as shape] [app.plugins.system-events :as se] [app.plugins.user :as user] @@ -416,7 +416,10 @@ (cb/with-objects (:objects page)) (cb/add-object shape))] - (st/emit! (ch/commit-changes changes) + ;; Track the commit until the renderer starts. + (st/emit! (ptk/data-event :text/reflow {:ids [(:id shape)] + :page-id (:id page)}) + (ch/commit-changes changes) (se/event plugin-id "create-shape" :type :text)) (when (features/active-feature? @st/state "render-wasm/v1") @@ -734,10 +737,5 @@ :waitForLayoutUpdate (fn [timeout] - ;; Always a promise, so a bad argument travels as a rejection. - (if (u/valid-timeout? timeout) - ;; Resolves once every shape with reflow work in flight has settled. - (wrf/wait-for-layout-update timeout) - (js/Promise. - (fn [_ reject] - (u/reject-not-valid reject :waitForLayoutUpdate timeout))))))) + ;; Resolves once every shape with reflow work in flight has settled. + (wrfp/wait-for-layout-update timeout)))) diff --git a/frontend/src/app/plugins/reflow.cljs b/frontend/src/app/plugins/reflow.cljs new file mode 100644 index 0000000000..306fff667b --- /dev/null +++ b/frontend/src/app/plugins/reflow.cljs @@ -0,0 +1,77 @@ +;; 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 app.plugins.reflow + "Promise adapter for the plugin `waitForLayoutUpdate` methods. Owns the + argument validation, the default deadline and the rejection shape; the + workspace only reports when its pending work has drained." + (:require + [app.common.data.macros :as dm] + [app.common.files.helpers :as cfh] + [app.common.uuid :as uuid] + [app.main.data.workspace.reflow :as wrf] + [beicon.v2.core :as rx])) + +;; Ceiling for callers that pass no timeout, so a pipeline that never drains +;; its marks rejects the promise rather than leaving it unsettled. +(def ^:private default-timeout 30000) + +;; Largest value a signed 32-bit timer accepts. +(def ^:private max-timeout 2147483647) + +(defn- valid-timeout? + "Checks that a plugin timeout fits a signed 32-bit timer." + [value] + (or (nil? value) + (and (number? value) + (pos? value) + (<= value max-timeout) + (js/Number.isFinite value)))) + +(defn- reject-invalid! + [reject value] + (let [msg (dm/str "[PENPOT PLUGIN] Value not valid: " value + ". Code: " :waitForLayoutUpdate)] + (.error js/console msg) + (reject (js/Error. msg)))) + +(defn shape-wait-ids + "Ids a per-shape wait covers: the shape subtree, its ancestors, and the file + its components sync from." + [objects file-id id] + (-> (into #{} (cfh/get-children-ids-with-self objects id)) + (into (cfh/get-parent-ids objects id)) + (conj file-id) + (disj uuid/zero))) + +(defn wait-for-layout-update + "Returns a JS Promise that resolves once every id in `ids` has drained from + the workspace pending map. A nil `ids` waits for every pending id; an empty + one has nothing to wait for and resolves right away. + + The promise is rejected when `timeout` (ms) is not a valid timer value, or + when it elapses first; a nil `timeout` uses `default-timeout`." + ([timeout] + (wait-for-layout-update nil timeout)) + ([ids timeout] + (js/Promise. + (fn [resolve reject] + (if-not (valid-timeout? timeout) + (reject-invalid! reject timeout) + ;; Race the settle signal against the deadline; the loser is + ;; unsubscribed. `settled` replays on subscribe, so an already drained + ;; map wins even against a 1ms deadline. + (->> (rx/race (->> (rx/of :timeout) + (rx/delay (or timeout default-timeout))) + (->> (wrf/settled ids) + (rx/map (constantly :ok)))) + (rx/take 1) + (rx/subs! + (fn [value] + (if (= value :timeout) + (reject (js/Error. "waitForLayoutUpdate timeout")) + (resolve))) + reject))))))) diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 88177b39ab..205f7d0d97 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -43,7 +43,6 @@ [app.main.data.workspace.guides :as dwgu] [app.main.data.workspace.interactions :as dwi] [app.main.data.workspace.libraries :as dwl] - [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.selection :as dws] [app.main.data.workspace.shape-layout :as dwsl] [app.main.data.workspace.shapes :as dwsh] @@ -58,6 +57,7 @@ [app.plugins.format :as format] [app.plugins.grid :as grid] [app.plugins.parser :as parser] + [app.plugins.reflow :as wrfp] [app.plugins.register :as r] [app.plugins.ruler-guides :as rg] [app.plugins.shadows :as shadows] @@ -1057,15 +1057,11 @@ :waitForLayoutUpdate (fn [timeout] - ;; Always a promise, so a bad argument travels as a rejection. - (if (u/valid-timeout? timeout) - ;; Resolves once the reflow work of this shape's subtree has - ;; settled: it can be marked on the shape or on its descendants. - (let [objects (u/locate-objects file-id page-id)] - (wrf/wait-for-layout-update (cfh/get-children-ids-with-self objects id) timeout)) - (js/Promise. - (fn [_ reject] - (u/reject-not-valid reject :waitForLayoutUpdate timeout))))) + ;; Wait for layout work that can affect this shape. + (let [objects (u/locate-objects file-id page-id)] + (wrfp/wait-for-layout-update + (wrfp/shape-wait-ids objects file-id id) + timeout))) ;; Plugin data :getPluginData diff --git a/frontend/src/app/plugins/text.cljs b/frontend/src/app/plugins/text.cljs index 3692ae1a59..f8e32458de 100644 --- a/frontend/src/app/plugins/text.cljs +++ b/frontend/src/app/plugins/text.cljs @@ -499,10 +499,10 @@ (u/not-valid plugin-id :growType "Cannot modify a page that is not currently active") :else - (st/emit! - (dwsh/update-shapes [id] #(assoc % :grow-type value)) - (when (features/active-feature? @st/state "render-wasm/v1") - (st/emit! (dwwt/resize-wasm-text-debounce id)))))))} + (do + (st/emit! (dwsh/update-shapes [id] #(assoc % :grow-type value))) + (when (features/active-feature? @st/state "render-wasm/v1") + (st/emit! (dwwt/resize-wasm-text-debounce id)))))))} {:name "fontId" :get #(-> % u/proxy->shape text-props :font-id format/format-mixed) diff --git a/frontend/src/app/plugins/utils.cljs b/frontend/src/app/plugins/utils.cljs index 21afd5cdde..49622d9710 100644 --- a/frontend/src/app/plugins/utils.cljs +++ b/frontend/src/app/plugins/utils.cljs @@ -291,14 +291,6 @@ (throw-not-valid code value) (display-not-valid code value))) -(defn valid-timeout? - "A plugin timeout argument: omitted, or a finite positive number of msecs." - [value] - (or (nil? value) - (and (number? value) - (pos? value) - (js/Number.isFinite value)))) - (defn reject-not-valid [reject code value] (let [msg (dm/str "[PENPOT PLUGIN] Value not valid: " value ". Code: " code)] diff --git a/frontend/src/app/render_wasm/api/fonts.cljs b/frontend/src/app/render_wasm/api/fonts.cljs index c3d5a32a35..eef60480fe 100644 --- a/frontend/src/app/render_wasm/api/fonts.cljs +++ b/frontend/src/app/render_wasm/api/fonts.cljs @@ -30,11 +30,30 @@ (def ^:private custom-fonts (l/derived :fonts st/state)) -;; Emits the font-id of every font whose glyphs wasm can already shape and -;; measure with. The browser-side loading of `app.main.fonts` is a separate -;; signal: it only says the DOM can render the font. +;; Emits every font face that WASM can measure. (defonce font-stored-stream (rx/subject)) +;; Emits failed font faces so layout can fall back. +(defonce font-storage-failed-stream (rx/subject)) + +;; Stores faces that currently use WASM fallbacks. +(defonce ^:private failed-font-data-keys (atom #{})) + +(defn font-data-key + "Returns the identity WASM uses to distinguish stored faces in one family." + [font-data] + (select-keys font-data [:font-id :weight :style :emoji?])) + +(defn- clear-font-storage-failure! + [font-data] + (swap! failed-font-data-keys disj (font-data-key font-data))) + +(defn- report-font-storage-failed! + [font-data] + (let [key (font-data-key font-data)] + (swap! failed-font-data-keys conj key) + (rx/push! font-storage-failed-stream key))) + (def ^:private default-font-size 14) (def ^:private default-line-height 1.2) (def ^:private default-letter-spacing 0.0) @@ -157,39 +176,91 @@ (:style font-data) emoji? fallback?) + (clear-font-storage-failure! font-data) ;; Reported after the store call: subscribers react by measuring text. - (rx/push! font-stored-stream (:font-id font-data)) + (rx/push! font-stored-stream (font-data-key font-data)) true))) -;; Tracks fonts currently being fetched: {url -> fallback?} -;; When the same font is requested as both primary and fallback, -;; the fallback flag is upgraded to true so it gets registered -;; in WASM's fallback_fonts set. +;; Tracks every font face waiting on each shared request. (def fetching (atom {})) +(defn- register-font-fetch! + [font-url font-data emoji? fallback?] + (let [key (font-data-key font-data)] + (clear-font-storage-failure! font-data) + (swap! fetching + update-in + [font-url key] + (fn [request] + {:font-data font-data + :emoji? emoji? + :fallback? (or fallback? (:fallback? request))})))) + +(defn- take-font-fetches! + [font-url] + (let [requests (vals (get @fetching font-url))] + (swap! fetching dissoc font-url) + requests)) + +(defn- fail-font-fetches! + [font-url cause] + (let [requests (take-font-fetches! font-url)] + (log/error :hint "Could not fetch font" + :font-url font-url + :cause cause) + (doseq [{:keys [font-data]} requests] + (report-font-storage-failed! font-data)))) + +(defn- store-font-fetch! + [body {:keys [font-data emoji? fallback?]}] + (try + (let [stored? (store-font-buffer font-data body emoji? fallback?)] + (when-not stored? + (report-font-storage-failed! font-data)) + stored?) + (catch :default cause + (log/error :hint "Could not store font" + :font-id (:font-id font-data) + :cause cause) + (report-font-storage-failed! font-data) + false))) + (defn- fetch-font [font-data font-url emoji? fallback?] - (if (contains? @fetching font-url) - (do (when fallback? (swap! fetching assoc font-url true)) - nil) + (cond + (nil? font-url) + ;; Fail missing font assets without sharing a nil request. (do - (swap! fetching assoc font-url fallback?) + (clear-font-storage-failure! font-data) + (tm/schedule #(report-font-storage-failed! font-data)) + nil) + + (contains? @fetching font-url) + (do + (register-font-fetch! font-url font-data emoji? fallback?) + nil) + + :else + (do + (register-font-fetch! font-url font-data emoji? fallback?) {:key font-url :callback (fn [] - (->> (http/send! {:method :get - :uri font-url - :response-type :buffer}) - (rx/map (fn [{:keys [body]}] - (let [fallback? (get @fetching font-url fallback?)] - (swap! fetching dissoc font-url) - (store-font-buffer font-data body emoji? fallback?)))) - (rx/catch (fn [cause] - (swap! fetching dissoc font-url) - (log/error :hint "Could not fetch font" - :font-url font-url - :cause cause) - (rx/empty)))))}))) + (try + (->> (http/send! {:method :get + :uri font-url + :response-type :buffer}) + (rx/map + (fn [{:keys [body]}] + (let [requests (take-font-fetches! font-url)] + (mapv (partial store-font-fetch! body) requests)))) + (rx/catch + (fn [cause] + (fail-font-fetches! font-url cause) + (rx/empty)))) + (catch :default cause + (fail-font-fetches! font-url cause) + (rx/empty))))}))) (defn- google-font-ttf-url [font-id font-variant-id font-weight font-style] @@ -220,9 +291,15 @@ (:style font-data) emoji?)))) +(defn font-ready? + "Returns true when WASM can lay out with the requested face or its fallback." + [font-data] + (or (contains? @failed-font-data-keys (font-data-key font-data)) + (font-stored? font-data (:emoji? font-data)))) + (defn- store-font-id [font-data asset-id emoji? fallback?] - (when asset-id + (if asset-id (let [uri (font-id->ttf-url (:font-id font-data) asset-id (:font-variant-id font-data) @@ -234,8 +311,16 @@ (if font-stored? ;; Deferred so consumers, which subscribe after dispatching the sync ;; that lands here, are listening when an already-stored font reports. - (tm/schedule #(rx/push! font-stored-stream (:font-id font-data))) - (fetch-font font-data uri emoji? fallback?))))) + (do + (clear-font-storage-failure! font-data) + (tm/schedule #(rx/push! font-stored-stream (font-data-key font-data)))) + (fetch-font font-data uri emoji? fallback?))) + ;; Report missing font assets asynchronously. + (do + (clear-font-storage-failure! font-data) + (tm/schedule + #(report-font-storage-failed! font-data)) + nil))) (defn serialize-font-style [font-style] diff --git a/frontend/test/frontend_tests/data/workspace_reflow_test.cljs b/frontend/test/frontend_tests/data/workspace_reflow_test.cljs index a1a50bf985..efbf9234e9 100644 --- a/frontend/test/frontend_tests/data/workspace_reflow_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_reflow_test.cljs @@ -6,13 +6,24 @@ (ns frontend-tests.data.workspace-reflow-test "Tests the reflow tasks the layout and text pipelines feed to - `app.main.data.workspace.reflow`, which is what plugin waits observe." + `app.main.data.workspace.reflow`, which is what plugin waits observe. The + promise view of the settle signal lives in `app.plugins.reflow`; these tests + use it because it is the wait the plugin API ships." (:require [app.common.uuid :as uuid] [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.shape-layout :as dwsl] + [app.main.data.workspace.shapes :as dwsh] [app.main.data.workspace.texts :as dwtxt] + [app.main.data.workspace.wasm-text :as dwwt] + [app.main.fonts :as fonts] + [app.plugins.reflow :as pwrf] + [app.render-wasm.api.fonts :as wasm.fonts] + [app.util.globals :as globals] + [app.util.http :as http] + [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] + [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) (t/use-fixtures :each {:before wrf/reset-pending! @@ -43,7 +54,7 @@ (t/async done (let [store (start-pipeline!)] (ptk/emit! store (ptk/data-event :layout/update {:ids [uuid/zero]})) - (-> (wrf/wait-for-layout-update nil 20) + (-> (pwrf/wait-for-layout-update nil 20) (.then #(t/is true "resolved with no pending work")) (.catch #(t/is false "a root-only update was marked as pending work")) (.then (fn [] @@ -59,16 +70,41 @@ _ (wrf/reset-pending!) current-task (wrf/start! :text-measure [id])] (wrf/finish! stale-task) - (-> (wrf/wait-for-layout-update [id] 20) + (-> (pwrf/wait-for-layout-update [id] 20) (.then #(t/is false "a stale completion drained current work")) (.catch #(t/is true "current work stayed pending")) (.then (fn [] (wrf/finish! current-task) - (wrf/wait-for-layout-update [id] 100))) + (pwrf/wait-for-layout-update [id] 100))) (.then #(t/is true "the exact current task drained normally")) (.catch #(t/is false "the current task did not drain")) (.then (fn [] (done))))))) +(t/deftest reinstalling-the-pending-scan-resets-work-and-keeps-tracking + ;; Reinstall the pending scan with the latest reducer. + (t/async done + (let [id (uuid/next) + stale (wrf/start! :text-measure [id]) + current* (atom nil)] + (#'wrf/install-pending-subscription!) + (-> (pwrf/wait-for-layout-update [id] 100) + (.then #(t/is true "reinstalling the scan reset its previous generation")) + (.catch #(t/is false "the replaced scan kept stale work pending")) + (.then + (fn [] + (reset! current* (wrf/start! :text-measure [id])) + (pwrf/wait-for-layout-update [id] 20))) + (.then #(t/is false "the replacement scan did not track new work")) + (.catch #(t/is true "the replacement scan tracked new work")) + (.then + (fn [] + (wrf/finish! stale) + (wrf/finish! @current*) + (pwrf/wait-for-layout-update [id] 100))) + (.then #(t/is true "the replacement scan drained its exact task")) + (.catch #(t/is false "the replacement scan did not drain")) + (.then (fn [] (done))))))) + (t/deftest pending-promise-finishes-at-the-operation-boundary ;; Imperative render work is pending from before its thunk starts until the ;; exact promise returned by that thunk settles; no timer is involved. @@ -82,12 +118,12 @@ (fn [] (reset! started? true) (js/Promise. (fn [resolve _] (reset! resolve* resolve))))) - (-> (wrf/wait-for-layout-update [id] 20) + (-> (pwrf/wait-for-layout-update [id] 20) (.then #(t/is false "resolved while the render operation was pending")) (.catch #(t/is @started? "the task was opened before running the operation")) (.then (fn [] (@resolve*) - (wrf/wait-for-layout-update [id] 100))) + (pwrf/wait-for-layout-update [id] 100))) (.then #(t/is true "resolved as soon as the render operation settled")) (.catch #(t/is false "the settled render operation stayed pending")) (.then (fn [] (done))))))) @@ -98,7 +134,7 @@ (wrf/run-pending! :text-measure [id] #(throw (js/Error. "boom"))) (catch :default _)) (t/async done - (-> (wrf/wait-for-layout-update [id] 100) + (-> (pwrf/wait-for-layout-update [id] 100) (.then #(t/is true "a synchronous failure drained its exact task")) (.catch #(t/is false "a synchronous failure leaked pending work")) (.then (fn [] (done))))))) @@ -110,10 +146,10 @@ task-a (wrf/start! :text-bridge [id-a]) task-b (wrf/start! :text-bridge [id-b])] (wrf/cancel-shapes! [id-a]) - (-> (wrf/wait-for-layout-update [id-a] 100) + (-> (pwrf/wait-for-layout-update [id-a] 100) (.then #(t/is true "deleted shape work was cancelled")) (.catch #(t/is false "deleted shape work stayed pending")) - (.then #(wrf/wait-for-layout-update [id-b] 20)) + (.then #(pwrf/wait-for-layout-update [id-b] 20)) (.then #(t/is false "cancelling one shape drained its sibling")) (.catch #(t/is true "sibling work stayed pending")) (.then (fn [] @@ -130,28 +166,290 @@ (ptk/emit! store (ptk/data-event :text/reflow {:ids [id-a id-b]})) (let [task-a (wrf/start! :text-measure [id-a])] (wrf/finish! task-a)) - (-> (wrf/wait-for-layout-update [id-b] 20) + (-> (pwrf/wait-for-layout-update [id-b] 20) (.then #(t/is false "the first text released its sibling bridge")) (.catch #(t/is true "the sibling bridge stayed pending")) (.then (fn [] (let [task-b (wrf/start! :text-measure [id-b])] (wrf/finish! task-b)) - (wrf/wait-for-layout-update [id-b] 100))) + (pwrf/wait-for-layout-update [id-b] 100))) (.then #(t/is true "the sibling drained after its own measurement")) (.catch #(t/is false "the sibling never drained")) (.then (fn [] (stop-text-pipeline! store) (done))))))) +(t/deftest text-bridge-observes-out-of-order-work + ;; Start all bridges before matching work can finish. + (t/async done + (let [store (start-text-pipeline!) + id-a (uuid/next) + id-b (uuid/next)] + (ptk/emit! store (ptk/data-event :text/reflow {:ids [id-a id-b]})) + (let [task-b (wrf/start! :text-measure [id-b])] + (wrf/finish! task-b)) + (let [task-a (wrf/start! :text-measure [id-a])] + (wrf/finish! task-a)) + (-> (pwrf/wait-for-layout-update [id-a id-b] 100) + (.then #(t/is true "both out-of-order bridges observed their work")) + (.catch #(t/is false "a bridge missed work that started out of order")) + (.then (fn [] + (stop-text-pipeline! store) + (done))))))) + +(t/deftest text-bridge-does-not-consume-preexisting-work + ;; Ignore matching work that started before the bridge. + (t/async done + (let [store (start-text-pipeline!) + id (uuid/next) + prior-task (wrf/start! :text-measure [id])] + (ptk/emit! store (ptk/data-event :text/reflow {:ids [id]})) + (wrf/finish! prior-task) + (-> (pwrf/wait-for-layout-update [id] 20) + (.then #(t/is false "preexisting work released the new bridge")) + (.catch #(t/is true "the new bridge remained pending")) + (.then (fn [] + (let [current-task (wrf/start! :text-measure [id])] + (wrf/finish! current-task)) + (pwrf/wait-for-layout-update [id] 100))) + (.then #(t/is true "work started after the bridge drained it")) + (.catch #(t/is false "the causal measurement did not drain the bridge")) + (.then (fn [] + (stop-text-pipeline! store) + (done))))))) + +(t/deftest cancelling-a-bridge-does-not-block-later-reflow-events + (t/async done + (let [store (start-text-pipeline!) + id-a (uuid/next) + id-b (uuid/next)] + (ptk/emit! store (ptk/data-event :text/reflow {:ids [id-a]})) + (wrf/cancel-shapes! [id-a]) + (ptk/emit! store (ptk/data-event :text/reflow {:ids [id-b]})) + (-> (pwrf/wait-for-layout-update [id-b] 20) + (.then #(t/is false "the later bridge was not opened")) + (.catch #(t/is true "the later bridge stayed pending")) + (.then (fn [] + (let [task-b (wrf/start! :text-measure [id-b])] + (wrf/finish! task-b)) + (pwrf/wait-for-layout-update [id-b] 100))) + (.then #(t/is true "the later bridge drained after its own work")) + (.catch #(t/is false "the cancelled bridge blocked the pipeline")) + (.then (fn [] + (stop-text-pipeline! store) + (done))))))) + +(t/deftest finalizing-a-page-cancels-its-text-bridges + (t/async done + (let [store (start-text-pipeline!) + id (uuid/next)] + (ptk/emit! store (ptk/data-event :text/reflow {:ids [id]})) + (ptk/emit! store (ptk/data-event :app.main.data.workspace.pages/finalize-page)) + (-> (pwrf/wait-for-layout-update [id] 100) + (.then #(t/is true "page teardown drained the unmeasured text bridge")) + (.catch #(t/is false "page teardown left text work pending")) + (.then (fn [] + (stop-text-pipeline! store) + (done))))))) + +(t/deftest failed-wasm-font-storage-falls-back-and-drains + (t/async done + (let [id (uuid/next) + font-key {:font-id "gfont-does-not-load" + :weight 400 + :style 0 + :emoji? false} + stream (rx/subject) + events (atom [])] + (->> (#'dwtxt/await-font-faces stream #{font-key} [id]) + (rx/subs! #(swap! events conj %))) + (#'wasm.fonts/report-font-storage-failed! font-key) + (-> (pwrf/wait-for-layout-update [id] 100) + (.then (fn [] + (t/is (= 1 (count @events)) + "font failure dispatches one fallback resize") + (t/is (wasm.fonts/font-ready? font-key) + "the resize gate accepts the failed face's fallback") + (done))) + (.catch (fn [_] + (t/is false "font failure leaked pending work") + (done))))))) + +(t/deftest failed-dom-font-load-falls-back-and-drains + (t/async done + (let [id (uuid/next) + font-id "gfont-layout-failure-test"] + (swap! fonts/fontsdb assoc font-id + {:id font-id + :backend :google + :family "Layout Failure Test" + :variants [{:id "regular"}]}) + (swap! fonts/loaded disj font-id) + (swap! fonts/loading dissoc font-id) + (mock/with-mocks + {globals/browser? (constantly true) + http/send! (fn [_] (rx/throw (js/Error. "font fetch failed")))} + (fn [done'] + (wrf/run-pending! :font [id] #(fonts/ensure-loaded! font-id)) + (-> (pwrf/wait-for-layout-update [id] 500) + (.then (fn [] + (t/is (not (contains? @fonts/loading font-id)) + "a failed load must not remain cached as loading"))) + (.catch #(t/is false "failed DOM font load leaked pending work")) + (.then (fn [] + (swap! fonts/fontsdb dissoc font-id) + (swap! fonts/loaded disj font-id) + (swap! fonts/loading dissoc font-id) + (done'))))) + done)))) + +(t/deftest failed-google-font-css-does-not-abort-shared-consumers + (t/async done + (let [font-id "gfont-optional-css-test" + values (atom []) + cleanup #(swap! fonts/fontsdb dissoc font-id)] + (swap! fonts/fontsdb assoc font-id + {:id font-id + :backend :google + :family "Optional CSS Test" + :variants [{:id "regular"}]}) + (mock/with-mocks + {http/send! (fn [_] (rx/throw (js/Error. "font css fetch failed")))} + (fn [done'] + (->> (fonts/fetch-font-css {:font-id font-id}) + (rx/subs! + #(swap! values conj %) + (fn [_] + (cleanup) + (t/is false "an optional font CSS failure escaped the shared helper") + (done')) + (fn [] + (cleanup) + (t/is (empty? @values) + "a failed optional font contributes no CSS") + (done'))))) + done)))) + +(t/deftest deduplicated-wasm-font-failure-settles-every-face + (t/async done + (let [font-url "https://example.test/shared-font.ttf" + regular {:font-id "gfont-shared-regular" + :weight 400 + :style 0 + :emoji? false} + bold {:font-id "gfont-shared-bold" + :weight 700 + :style 0 + :emoji? false}] + (mock/with-mocks + {http/send! (fn [_] (rx/throw (js/Error. "shared fetch failed")))} + (fn [done'] + (let [request (#'wasm.fonts/fetch-font regular font-url false false) + duplicate (#'wasm.fonts/fetch-font bold font-url false false)] + (t/is (some? request) "the first face owns the shared fetch") + (t/is (nil? duplicate) "the second face reuses the shared fetch") + (->> ((:callback request)) + (rx/subs! + (fn [_]) + (fn [_] + (t/is false "the shared fetch failure escaped its fallback") + (done')) + (fn [] + (t/is (wasm.fonts/font-ready? regular) + "the first face settled through fallback") + (t/is (wasm.fonts/font-ready? bold) + "the deduplicated face settled through fallback") + (done')))))) + done)))) + +(t/deftest missing-wasm-font-url-settles-without-entering-fetch-map + (t/async done + (let [font-data {:font-id "gfont-missing-url" + :weight 400 + :style 0 + :emoji? false}] + (t/is (nil? (#'wasm.fonts/fetch-font font-data nil false false)) + "a missing URL starts no request") + (t/is (not (contains? @wasm.fonts/fetching nil)) + "missing URLs are not deduplicated under nil") + (js/setTimeout + (fn [] + (t/is (wasm.fonts/font-ready? font-data) + "the missing face settled through fallback") + (done)) + 0)))) + +(t/deftest wasm-font-resize-waits-for-every-face + (t/async done + (let [id (uuid/next) + regular-key {:font-id "gfont-mixed" + :weight 400 + :style 0 + :emoji? false} + bold-key {:font-id "gfont-mixed" + :weight 700 + :style 0 + :emoji? false} + stream (rx/subject) + events (atom [])] + (->> (#'dwtxt/await-font-faces stream #{regular-key bold-key} [id]) + (rx/subs! #(swap! events conj %))) + (rx/push! wasm.fonts/font-stored-stream regular-key) + (-> (pwrf/wait-for-layout-update [id] 20) + (.then #(t/is false "the first face released the font task")) + (.catch #(t/is true "the second face remained pending")) + (.then (fn [] + (rx/push! wasm.fonts/font-storage-failed-stream bold-key) + (pwrf/wait-for-layout-update [id] 100))) + (.then (fn [] + (t/is (= 1 (count @events)) + "all faces settling dispatches exactly one resize") + (done))) + (.catch (fn [_] + (t/is false "the complete face set did not drain") + (done))))))) + +(t/deftest buffered-wasm-resize-releases-on-stop-without-a-commit + (t/async done + (let [store (ptk/store {:state {} :on-error #(js/console.error %)}) + id (uuid/next)] + (ptk/emit! store (dwsh/update-shapes-buffer-start)) + (ptk/emit! store (dwwt/resize-wasm-text-all [id])) + (-> (pwrf/wait-for-layout-update [id] 20) + (.then #(t/is false "the buffered resize was not marked pending")) + (.catch #(t/is true "the resize stayed pending while the buffer was open")) + (.then (fn [] + (ptk/emit! store (dwsh/update-shapes-buffer-stop)) + (pwrf/wait-for-layout-update [id] 500))) + (.then #(t/is true "buffer stop released the fallback resize")) + (.catch #(t/is false "buffer stop without a commit leaked pending work")) + (.then (fn [] (done))))))) + +(t/deftest buffered-wasm-resize-releases-on-workspace-finalize + (t/async done + (let [store (ptk/store {:state {} :on-error #(js/console.error %)}) + id (uuid/next)] + (ptk/emit! store (dwsh/update-shapes-buffer-start)) + (ptk/emit! store (dwwt/resize-wasm-text-all [id])) + (-> (pwrf/wait-for-layout-update [id] 20) + (.then #(t/is false "the buffered resize was not marked pending")) + (.catch #(t/is true "the resize stayed pending while the buffer was open")) + (.then (fn [] + (ptk/emit! store (ptk/data-event :app.main.data.workspace/finalize-workspace)) + (pwrf/wait-for-layout-update [id] 500))) + (.then #(t/is true "workspace finalization released the buffered resize")) + (.catch #(t/is false "workspace finalization leaked pending work")) + (.then (fn [] (done))))))) + (t/deftest layout-update-is-pending-until-the-buffer-flushes ;; A shape id is marked on arrival and drained when the update is processed. (t/async done (let [store (start-pipeline!)] (ptk/emit! store (ptk/data-event :layout/update {:ids [(uuid/next) uuid/zero]})) - (-> (wrf/wait-for-layout-update nil 20) + (-> (pwrf/wait-for-layout-update nil 20) (.then #(t/is false "resolved while the update was still buffered")) (.catch #(t/is true "stayed pending until the flush")) - (.then #(wrf/wait-for-layout-update nil 5000)) + (.then #(pwrf/wait-for-layout-update nil 5000)) (.then #(t/is true "resolved once the update was processed")) (.catch #(t/is false "the pipeline never drained its mark")) (.then (fn [] diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index fbbf852c82..207b295a11 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -164,169 +164,151 @@ :team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9"}) (t/deftest expired-organization-sso-navigates-to-identity-provider - (t/testing "the browser is sent to the identity provider instead of an error page" - (let [events (atom [])] - (with-redefs [rp/cmd! - (mock/stub - (fn [_command _params] - (rx/of {:authorized false - :redirect-uri "https://idp.example.com/authorize"}))) - - rt/get-current-href - (constantly workspace-href) - - st/emit! - (mock/stub (fn [& emitted] (swap! events into emitted)))] - - (errors/on-error (sso-required-error)) - - (t/is (= [::rt/nav-raw] (mapv ptk/type @events))))))) + (t/async done + (t/testing "the browser is sent to the identity provider instead of an error page" + (let [events (atom [])] + (mock/with-mocks + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + rt/get-current-href (constantly workspace-href) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} + (fn [done'] + (errors/on-error (sso-required-error)) + (t/is (= [::rt/nav-raw] (mapv ptk/type @events))) + (done')) + done))))) (t/deftest expired-organization-sso-comes-back-to-the-current-location - (t/testing "the SSO check asks the provider to return the user where they were" - (let [rpc-calls (atom [])] - (with-redefs [rp/cmd! - (mock/stub - (fn [command params] - (swap! rpc-calls conj {:command command :params params}) - (rx/of {:authorized false - :redirect-uri "https://idp.example.com/authorize"}))) - - rt/get-current-href - (constantly workspace-href) - - st/emit! mock/noop] - - (errors/on-error (sso-required-error)) - - (t/is (= [{:command :check-nitrate-sso - :params {:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9" - :organization-id organization-id - :url workspace-href}}] - @rpc-calls)))))) + (t/async done + (t/testing "the SSO check asks the provider to return the user where they were" + (let [rpc-calls (atom [])] + (mock/with-mocks + {rp/cmd! (mock/stub + (fn [command params] + (swap! rpc-calls conj {:command command :params params}) + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + rt/get-current-href (constantly workspace-href) + st/emit! mock/noop} + (fn [done'] + (errors/on-error (sso-required-error)) + (t/is (= [{:command :check-nitrate-sso + :params {:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9" + :organization-id organization-id + :url workspace-href}}] + @rpc-calls)) + (done')) + done))))) (t/deftest already-satisfied-organization-sso-retries-the-location - (t/testing "a session renewed meanwhile (e.g. in another tab) reloads instead of erroring" - (let [events (atom [])] - (with-redefs [rp/cmd! - (mock/stub - (fn [_command _params] - (rx/of {:authorized true :reason :sso-satisfied}))) - - rt/get-current-href - (constantly workspace-href) - - st/emit! - (mock/stub (fn [& emitted] (swap! events into emitted)))] - - (errors/on-error (sso-required-error)) - - (t/is (= [::rt/reload] (mapv ptk/type @events))))))) + (t/async done + (t/testing "a session renewed meanwhile (e.g. in another tab) reloads instead of erroring" + (let [events (atom [])] + (mock/with-mocks + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized true :reason :sso-satisfied}))) + rt/get-current-href (constantly workspace-href) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} + (fn [done'] + (errors/on-error (sso-required-error)) + (t/is (= [::rt/reload] (mapv ptk/type @events))) + (done')) + done))))) (t/deftest organization-sso-without-usable-provider-shows-the-sso-error-dialog - (t/testing "SSO is required but there is nowhere to go: offer a retry, not a permission error" - (let [assigned* (atom nil)] - (with-redefs [rp/cmd! - (mock/stub - (fn [_command _params] - (rx/of {:authorized false :redirect-uri nil}))) - - rt/get-current-href - (constantly workspace-href) - - rt/assign-exception - (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))] - - (errors/on-error (sso-required-error)) - - (t/is (= :sso-error (:type @assigned*))) - (t/is (= organization-id (:organization-id @assigned*))) - (t/is (true? (:is-workspace @assigned*))))))) + (t/async done + (t/testing "SSO is required but there is nowhere to go: offer a retry, not a permission error" + (let [assigned* (atom nil)] + (mock/with-mocks + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized false :redirect-uri nil}))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (fn [done'] + (errors/on-error (sso-required-error)) + (t/is (= :sso-error (:type @assigned*))) + (t/is (= organization-id (:organization-id @assigned*))) + (t/is (true? (:is-workspace @assigned*))) + (done')) + done))))) (t/deftest organization-sso-without-team-access-reports-a-permission-failure - (t/testing "a user who cannot reach the team keeps getting the authentication error" - (let [assigned* (atom nil)] - (with-redefs [rp/cmd! - (mock/stub - (fn [_command _params] - (rx/of {:authorized true :reason :no-team-access}))) - - rt/get-current-href - (constantly workspace-href) - - rt/assign-exception - (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))] - - (errors/on-error (sso-required-error)) - - (t/is (= :authentication (:type @assigned*))) - (t/is (= :nitrate-sso-required (:code @assigned*))))))) + (t/async done + (t/testing "a user who cannot reach the team keeps getting the authentication error" + (let [assigned* (atom nil)] + (mock/with-mocks + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized true :reason :no-team-access}))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (fn [done'] + (errors/on-error (sso-required-error)) + (t/is (= :authentication (:type @assigned*))) + (t/is (= :nitrate-sso-required (:code @assigned*))) + (done')) + done))))) (t/deftest organization-sso-does-not-retry-on-an-unexplained-authorization - (t/testing "reloading on an answer we don't understand would spin on the same rejection" - (let [events (atom [])] - (with-redefs [rp/cmd! - (mock/stub (fn [_command _params] (rx/of {:authorized true}))) - - rt/get-current-href - (constantly workspace-href) - - rt/assign-exception - (fn [error] (ptk/data-event ::assigned error)) - - ;; async-emit! is variadic-only, so the replacement must be - ;; variadic too for the compiled static dispatch to find it - st/async-emit! - (fn [& emitted] (swap! events into emitted))] - - (errors/on-error (sso-required-error)) - - (t/is (= [::assigned] (mapv ptk/type @events))))))) + (t/async done + (t/testing "reloading on an answer we don't understand would spin on the same rejection" + (let [events (atom [])] + (mock/with-mocks + {rp/cmd! (mock/stub (fn [_command _params] (rx/of {:authorized true}))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] (ptk/data-event ::assigned error)) + st/async-emit! (fn [& emitted] (swap! events into emitted))} + (fn [done'] + (errors/on-error (sso-required-error)) + (t/is (= [::assigned] (mapv ptk/type @events))) + (done')) + done))))) (t/deftest organization-sso-error-without-context-is-reported-as-it-arrives - (t/testing "with no organization and no team there is nothing to check" - (let [rpc-calls (atom 0) - assigned* (atom nil)] - (with-redefs [rp/cmd! - (mock/stub (fn [_command _params] - (swap! rpc-calls inc) - (rx/empty))) - - rt/get-current-href - (constantly workspace-href) - - rt/assign-exception - (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))] - - (errors/on-error {:type :authentication - :code :nitrate-sso-required}) - - (t/is (zero? @rpc-calls)) - (t/is (= :nitrate-sso-required (:code @assigned*))))))) + (t/async done + (t/testing "with no organization and no team there is nothing to check" + (let [rpc-calls (atom 0) + assigned* (atom nil)] + (mock/with-mocks + {rp/cmd! (mock/stub (fn [_command _params] + (swap! rpc-calls inc) + (rx/empty))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (fn [done'] + (errors/on-error {:type :authentication + :code :nitrate-sso-required}) + (t/is (zero? @rpc-calls)) + (t/is (= :nitrate-sso-required (:code @assigned*))) + (done')) + done))))) (t/deftest a-resultless-organization-sso-check-does-not-wedge-later-rejections - (t/testing "the one-in-flight guard is released even when no answer arrives" - (let [rpc-calls (atom 0)] - (with-redefs [rp/cmd! - (mock/stub (fn [_command _params] - (swap! rpc-calls inc) - (rx/empty))) - - rt/get-current-href - (constantly workspace-href) - - st/emit! mock/noop] - - (errors/on-error (sso-required-error)) - (errors/on-error (sso-required-error)) - - (t/is (= 2 @rpc-calls)))))) + (t/async done + (t/testing "the one-in-flight guard is released even when no answer arrives" + (let [rpc-calls (atom 0)] + (mock/with-mocks + {rp/cmd! (mock/stub (fn [_command _params] + (swap! rpc-calls inc) + (rx/empty))) + rt/get-current-href (constantly workspace-href) + st/emit! mock/noop} + (fn [done'] + (errors/on-error (sso-required-error)) + (errors/on-error (sso-required-error)) + (t/is (= 2 @rpc-calls)) + (done')) + done))))) ;; A failing check must stay a failing check: the generic handling turns it ;; into a toast, whereas swallowing it would show a permission error for diff --git a/frontend/test/frontend_tests/plugins/comments_test.cljs b/frontend/test/frontend_tests/plugins/comments_test.cljs index ee19153a73..519c8581fa 100644 --- a/frontend/test/frontend_tests/plugins/comments_test.cljs +++ b/frontend/test/frontend_tests/plugins/comments_test.cljs @@ -17,44 +17,54 @@ (def ^:private plugin-id "00000000-0000-0000-0000-000000000000") (t/deftest comment-thread-remove-allows-the-owner - (let [owner-id (random-uuid) - file-id (random-uuid) - page-id (random-uuid) - thread-id (random-uuid) - emitted (atom nil) - thread (comments/comment-thread-proxy - plugin-id - file-id - page-id - {:id thread-id :owner-id owner-id})] - (set! st/state (atom {:profile {:id owner-id}})) - (with-redefs [r/check-permission (constantly true) - dc/delete-comment-thread-on-workspace - (mock/stub (fn [params callback] - (callback) - [:delete-thread params])) - st/emit! (mock/stub (fn [event] (reset! emitted event)))] - (let [result (.remove thread)] - (t/is (instance? js/Promise result)) - (t/is (= [:delete-thread {:id thread-id}] @emitted)))))) + (t/async done + (let [owner-id (random-uuid) + file-id (random-uuid) + page-id (random-uuid) + thread-id (random-uuid) + emitted (atom nil) + thread (comments/comment-thread-proxy + plugin-id + file-id + page-id + {:id thread-id :owner-id owner-id})] + (set! st/state (atom {:profile {:id owner-id}})) + (mock/with-mocks + {r/check-permission (constantly true) + dc/delete-comment-thread-on-workspace + (mock/stub (fn [params callback] + (callback) + [:delete-thread params])) + st/emit! (mock/stub (fn [event] (reset! emitted event)))} + (fn [done'] + (let [result (.remove thread)] + (t/is (instance? js/Promise result)) + (t/is (= [:delete-thread {:id thread-id}] @emitted)) + (done'))) + done)))) (t/deftest page-remove-comment-thread-emits-delete-event - (let [file-id (random-uuid) - page-id (random-uuid) - thread-id (random-uuid) - emitted (atom nil) - page (page/page-proxy plugin-id file-id page-id) - thread (comments/comment-thread-proxy - plugin-id - file-id - page-id - {:id thread-id :owner-id (random-uuid)})] - (with-redefs [r/check-permission (constantly true) - dc/delete-comment-thread-on-workspace - (mock/stub (fn [params callback] - (callback) - [:delete-thread params])) - st/emit! (mock/stub (fn [event] (reset! emitted event)))] - (let [result (.removeCommentThread page thread)] - (t/is (instance? js/Promise result)) - (t/is (= [:delete-thread {:id thread-id}] @emitted)))))) + (t/async done + (let [file-id (random-uuid) + page-id (random-uuid) + thread-id (random-uuid) + emitted (atom nil) + page (page/page-proxy plugin-id file-id page-id) + thread (comments/comment-thread-proxy + plugin-id + file-id + page-id + {:id thread-id :owner-id (random-uuid)})] + (mock/with-mocks + {r/check-permission (constantly true) + dc/delete-comment-thread-on-workspace + (mock/stub (fn [params callback] + (callback) + [:delete-thread params])) + st/emit! (mock/stub (fn [event] (reset! emitted event)))} + (fn [done'] + (let [result (.removeCommentThread page thread)] + (t/is (instance? js/Promise result)) + (t/is (= [:delete-thread {:id thread-id}] @emitted)) + (done'))) + done)))) diff --git a/frontend/test/frontend_tests/plugins/context_shapes_test.cljs b/frontend/test/frontend_tests/plugins/context_shapes_test.cljs index c417b43dab..e6c5d2d629 100644 --- a/frontend/test/frontend_tests/plugins/context_shapes_test.cljs +++ b/frontend/test/frontend_tests/plugins/context_shapes_test.cljs @@ -11,9 +11,11 @@ [app.common.uuid :as uuid] [app.main.data.workspace.reflow :as wrf] [app.main.data.workspace.shapes :as dwsh] + [app.main.data.workspace.texts :as dwtxt] [app.main.data.workspace.wasm-text :as dwwt] [app.main.store :as st] [app.plugins.api :as api] + [app.plugins.reflow :as pwrf] [app.plugins.shape :as shape] [app.util.object :as obj] [beicon.v2.core :as rx] @@ -445,6 +447,24 @@ (set! st/stream (ptk/input-stream test-store)) test-store)) +(t/deftest test-update-shapes-invokes-update-function-once + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1) + {:renderer :svg}) + _ (set! st/state store) + _ (set! st/stream (ptk/input-stream store)) + ^js ctx (api/create-context zero-id) + ^js rect (.createRectangle ctx) + id (obj/get rect "$id") + calls (atom 0)] + (ptk/emit! store + (dwsh/update-shapes + [id] + (fn [shape] + (swap! calls inc) + (assoc shape :opacity 0.5)))) + (t/is (= 1 @calls) "the update function ran once for the committed shape") + (t/is (= 0.5 (.-opacity rect)) "the single computed result was committed"))) + (t/deftest test-wait-for-layout-update-no-pending ;; When nothing is pending the promise resolves immediately via the fast path ;; (the behavior-subject replays the empty map on subscribe). @@ -459,6 +479,209 @@ (t/is false (str "unexpected rejection: " err)) (done))))))) +(t/deftest test-create-text-bridges-dom-measurement + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1) + {:renderer :svg})] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (ptk/emit! store (dwtxt/initialize-text-reflow)) + (let [^js ctx (api/create-context zero-id) + ^js text (.createText ctx "Measure me") + id (obj/get text "$id")] + (-> (.waitForLayoutUpdate ctx 20) + (.then #(t/is false "createText resolved before DOM measurement started")) + (.catch #(t/is true "createText stayed bridged to DOM measurement")) + (.then (fn [] + (let [task (wrf/start! :text-measure [id])] + (wrf/finish! task)) + (.waitForLayoutUpdate ctx 100))) + (.then #(t/is true "the bridge drained after measurement started")) + (.catch #(t/is false "the createText bridge did not drain")) + (.then (fn [] + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (done)))))))) + +(t/deftest test-dom-position-data-stays-pending-until-commit + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1) + {:renderer :svg})] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (ptk/emit! store (dwtxt/initialize-text-reflow)) + (let [^js ctx (api/create-context zero-id) + ^js text (.createText ctx "Position me") + id (obj/get text "$id") + task (wrf/start! :text-measure [id]) + position-data + [{:x 10 :y 20 :width 30 :height 12}]] + (wrf/finish! task) + (-> (.waitForLayoutUpdate text 100) + (.then + (fn [] + (ptk/emit! store (dwtxt/update-position-data id position-data)) + (-> (.waitForLayoutUpdate text 20) + (.then (constantly false)) + (.catch (constantly true))))) + (.then + (fn [timed-out?] + (t/is timed-out? + "position data stayed pending across its debounce") + (.waitForLayoutUpdate text 500))) + (.then + (fn [] + (let [bounds (.-textBounds text)] + (t/is (= 30 (obj/get bounds "width")) + "the wait exposed the committed text bounds")) + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (done))) + (.catch + (fn [cause] + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (t/is false (str "position-data wait did not settle: " cause)) + (done)))))))) + +(t/deftest test-buffered-text-update-bridges-dom-measurement + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1) + {:renderer :svg})] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (ptk/emit! store (dwtxt/initialize-text-reflow)) + (let [^js ctx (api/create-context zero-id) + ^js text (.createText ctx "Before") + id (obj/get text "$id") + task (wrf/start! :text-measure [id])] + (wrf/finish! task) + (-> (.waitForLayoutUpdate text 100) + (.then + (fn [] + (ptk/emit! store (dwsh/update-shapes-buffer-start)) + (set! (.-characters text) "After") + (ptk/emit! store (dwsh/update-shapes-buffer-stop)) + (.waitForLayoutUpdate text 20))) + (.then #(t/is false "buffered update resolved before DOM measurement")) + (.catch #(t/is true "buffered update stayed bridged to DOM measurement")) + (.then + (fn [] + (let [task (wrf/start! :text-measure [id])] + (wrf/finish! task)) + (.waitForLayoutUpdate text 100))) + (.then #(t/is true "the buffered update bridge drained")) + (.catch #(t/is false "the buffered update bridge did not drain")) + (.then + (fn [] + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (done)))))))) + +(t/deftest test-wasm-grow-type-wait-observes-its-resize + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1))] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (ptk/emit! store (dwtxt/initialize-text-reflow)) + (let [^js ctx (api/create-context zero-id) + ^js text (.createText ctx "Resize after grow type")] + (-> (.waitForLayoutUpdate text 500) + (.then + (fn [] + (set! (.-growType text) "fixed") + (.waitForLayoutUpdate text 500))) + (.then + (fn [] + (t/is (= "fixed" (.-growType text)) + "the grow-type bridge drained after its WASM resize") + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (done))) + (.catch + (fn [cause] + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (t/is false (str "grow-type wait did not settle: " cause)) + (done)))))))) + +(t/deftest test-cloned-text-bridges-dom-measurement + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1) + {:renderer :svg})] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (ptk/emit! store (dwtxt/initialize-text-reflow)) + (let [^js ctx (api/create-context zero-id) + ^js text (.createText ctx "Clone me") + id (obj/get text "$id") + task (wrf/start! :text-measure [id])] + (wrf/finish! task) + (-> (.waitForLayoutUpdate text 100) + (.then + (fn [] + (let [^js clone (.clone text) + clone-id (obj/get clone "$id")] + (-> (.waitForLayoutUpdate clone 20) + (.then #(t/is false "clone resolved before DOM measurement")) + (.catch #(t/is true "clone stayed bridged to DOM measurement")) + (.then + (fn [] + (let [task (wrf/start! :text-measure [clone-id])] + (wrf/finish! task)) + (.waitForLayoutUpdate clone 100))))))) + (.then #(t/is true "the cloned text bridge drained")) + (.catch #(t/is false "the cloned text bridge did not drain")) + (.then + (fn [] + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (done)))))))) + +(t/deftest test-fixed-text-resize-bridges-dom-measurement + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1) + {:renderer :svg})] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (ptk/emit! store (dwtxt/initialize-text-reflow)) + (let [^js ctx (api/create-context zero-id) + ^js text (.createText ctx "Resize me") + id (obj/get text "$id") + task (wrf/start! :text-measure [id])] + (wrf/finish! task) + (-> (.waitForLayoutUpdate text 100) + (.then + (fn [] + ;; Finish the grow-type update before resizing. + (set! (.-growType text) "fixed") + (let [task (wrf/start! :text-measure [id])] + (wrf/finish! task)) + (.waitForLayoutUpdate text 100))) + (.then + (fn [] + (.resize text 240 80) + ;; Turn only this short wait into a boolean. + (-> (.waitForLayoutUpdate text 20) + (.then (fn [] false)) + (.catch (fn [_] true))))) + (.then + (fn [timed-out?] + (t/is timed-out? + "fixed text resize stayed bridged to DOM measurement") + (let [task (wrf/start! :text-measure [id])] + (wrf/finish! task)) + (.waitForLayoutUpdate text 100))) + (.then + (fn [] + (t/is true "the resize bridge drained after measurement") + ;; Match the DOM renderer's 0.001 geometry tolerance. + (.resize text 240.0005 80) + (.waitForLayoutUpdate text 100))) + (.then + (fn [] + (t/is true "a sub-tolerance resize opened no DOM bridge") + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (done))) + (.catch + (fn [cause] + (ptk/emit! store (dwtxt/finalize-text-reflow)) + (t/is false (str "unexpected resize bridge rejection: " cause)) + (done)))))))) + (t/deftest test-wait-for-layout-update-pending ;; While a shape is pending the context promise stays unresolved; it resolves ;; once that shape is marked done. @@ -584,6 +807,54 @@ 20)) 20)))))) +(t/deftest test-wait-for-layout-update-ancestor + ;; A shape wait also covers layout on its parents. + (t/async done + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1))] + (set! st/state store) + (set! st/stream (ptk/input-stream store)) + (let [^js ctx (api/create-context zero-id) + ^js board (.createBoard ctx) + ^js rect (.createRectangle ctx)] + (.appendChild board rect) + (let [board-id (obj/get board "$id") + task (wrf/start! :layout [board-id]) + resolved (atom false)] + (-> (.waitForLayoutUpdate rect) + (.then (fn [] (reset! resolved true))) + (.catch (fn [err] + (t/is false (str "unexpected rejection: " err))))) + (js/setTimeout + (fn [] + (t/is (false? @resolved) "child wait must block on a pending ancestor") + (wrf/finish! task) + (js/setTimeout + (fn [] + (t/is (true? @resolved) "resolves once the ancestor drains") + (done)) + 20)) + 20)))))) + +(t/deftest test-shape-wait-observes-file-sync + (t/async done + (let [file (cthf/sample-file :file1 :page-label :page1) + store (ths/setup-store file) + _ (set! st/state store) + _ (set! st/stream (ptk/input-stream store)) + ^js ctx (api/create-context zero-id) + ^js shape (.createRectangle ctx) + task (wrf/start! :sync-file [(:id file)])] + (-> (.waitForLayoutUpdate shape 20) + (.then #(t/is false "shape wait ignored its pending file sync")) + (.catch + (fn [] + (t/is true "shape wait remained pending for its file sync") + (wrf/finish! task) + (.waitForLayoutUpdate shape 100))) + (.then #(t/is true "shape wait drained after the file sync")) + (.catch #(t/is false "shape wait did not drain its file sync")) + (.then (fn [] (done))))))) + (t/deftest test-wait-for-layout-update-invalid-timeout ;; A non-numeric or non-positive timeout is an invalid argument. The method ;; always hands back a promise and rejects it, whatever the plugin's @@ -603,6 +874,7 @@ (rejected? (.waitForLayoutUpdate ctx -5)) (rejected? (.waitForLayoutUpdate ctx js/NaN)) (rejected? (.waitForLayoutUpdate ctx js/Infinity)) + (rejected? (.waitForLayoutUpdate ctx 2147483648)) (rejected? (.waitForLayoutUpdate shape "soon"))]) (.then (fn [results] (t/is (every? true? (array-seq results)) @@ -661,7 +933,7 @@ resolved (atom false)] (ptk/emit! store (dwsh/update-shapes-buffer-start)) (ptk/emit! store (dwwt/resize-wasm-text-all [id])) - (-> (wrf/wait-for-layout-update [id] nil) + (-> (pwrf/wait-for-layout-update [id] nil) (.then (fn [] (reset! resolved true))) (.catch (fn [err] (t/is false (str "unexpected rejection: " err))))) diff --git a/frontend/test/frontend_tests/ui/routes_test.cljs b/frontend/test/frontend_tests/ui/routes_test.cljs index 3ebb7edbbe..ad52f2fb05 100644 --- a/frontend/test/frontend_tests/ui/routes_test.cljs +++ b/frontend/test/frontend_tests/ui/routes_test.cljs @@ -23,63 +23,75 @@ :query-params {:team-id (str team-id)}}) (t/deftest sso-check-is-cached-for-five-minutes - (let [team-id (uuid/next) - match (workspace-match team-id) - now (atom (ct/inst "2026-08-11T10:00:00Z")) - rpc-calls (atom 0) - events (atom [])] - (with-redefs [cf/flags (conj cf/flags :admin-console) - ct/now (mock/stub (fn [] @now)) - rp/cmd! (mock/stub - (fn [command params] - (t/is (= :check-nitrate-sso command)) - (t/is (= team-id (:team-id params))) - (swap! rpc-calls inc) - (rx/of {:authorized true}))) - st/emit! (mock/stub - (fn [& emitted] - (swap! events into emitted)))] - (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") - (reset! now (ct/plus @now #js {:minutes 4 :seconds 59})) - (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") - - (t/is (= 1 @rpc-calls)) - (t/is (= 2 (count @events)))))) + (t/async done + (let [team-id (uuid/next) + match (workspace-match team-id) + now (atom (ct/inst "2026-08-11T10:00:00Z")) + rpc-calls (atom 0) + events (atom [])] + (mock/with-mocks + {cf/flags (conj cf/flags :admin-console) + ct/now (mock/stub (fn [] @now)) + rp/cmd! (mock/stub + (fn [command params] + (t/is (= :check-nitrate-sso command)) + (t/is (= team-id (:team-id params))) + (swap! rpc-calls inc) + (rx/of {:authorized true}))) + st/emit! (mock/stub + (fn [& emitted] + (swap! events into emitted)))} + (fn [done'] + (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") + (reset! now (ct/plus @now #js {:minutes 4 :seconds 59})) + (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") + (t/is (= 1 @rpc-calls)) + (t/is (= 2 (count @events))) + (done')) + done)))) (t/deftest sso-check-is-refreshed-after-five-minutes - (let [team-id (uuid/next) - match (workspace-match team-id) - now (atom (ct/inst "2026-08-11T10:00:00Z")) - rpc-calls (atom 0)] - (with-redefs [cf/flags (conj cf/flags :admin-console) - ct/now (mock/stub (fn [] @now)) - rp/cmd! (mock/stub - (fn [_ _] - (swap! rpc-calls inc) - (rx/of {:authorized true}))) - st/emit! mock/noop] - (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") - (reset! now (ct/plus @now #js {:minutes 5})) - (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") - - (t/is (= 2 @rpc-calls))))) + (t/async done + (let [team-id (uuid/next) + match (workspace-match team-id) + now (atom (ct/inst "2026-08-11T10:00:00Z")) + rpc-calls (atom 0)] + (mock/with-mocks + {cf/flags (conj cf/flags :admin-console) + ct/now (mock/stub (fn [] @now)) + rp/cmd! (mock/stub + (fn [_ _] + (swap! rpc-calls inc) + (rx/of {:authorized true}))) + st/emit! mock/noop} + (fn [done'] + (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") + (reset! now (ct/plus @now #js {:minutes 5})) + (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") + (t/is (= 2 @rpc-calls)) + (done')) + done)))) (t/deftest sso-redirect-result-is-not-cached - (let [team-id (uuid/next) - match (workspace-match team-id) - rpc-calls (atom 0) - events (atom [])] - (with-redefs [cf/flags (conj cf/flags :admin-console) - rp/cmd! (mock/stub - (fn [_ _] - (swap! rpc-calls inc) - (rx/of {:authorized false - :redirect-uri "https://idp.example.com/authorize"}))) - st/emit! (mock/stub - (fn [& emitted] - (swap! events into emitted)))] - (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") - (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") - - (t/is (= 2 @rpc-calls)) - (t/is (= 2 (count @events)))))) + (t/async done + (let [team-id (uuid/next) + match (workspace-match team-id) + rpc-calls (atom 0) + events (atom [])] + (mock/with-mocks + {cf/flags (conj cf/flags :admin-console) + rp/cmd! (mock/stub + (fn [_ _] + (swap! rpc-calls inc) + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + st/emit! (mock/stub + (fn [& emitted] + (swap! events into emitted)))} + (fn [done'] + (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") + (#'routes/check-sso-and-navigate match true "https://penpot.example.com/#/workspace") + (t/is (= 2 @rpc-calls)) + (t/is (= 2 (count @events))) + (done')) + done)))) diff --git a/plugins/apps/plugin-api-test-suite/src/tests/wait-layout.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/wait-layout.test.ts index 79c0fb330d..c2b67a4d5b 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/wait-layout.test.ts +++ b/plugins/apps/plugin-api-test-suite/src/tests/wait-layout.test.ts @@ -1,6 +1,13 @@ import { expect, expectReject } from '../framework/expect'; import { describe, test } from '../framework/registry'; -import type { Board, Font, Group, Shape, Text } from '@penpot/plugin-types'; +import type { + Board, + Font, + Group, + Penpot, + Shape, + Text, +} from '@penpot/plugin-types'; import type { TestContext } from '../framework/types'; // waitForLayoutUpdate (context-level and per-shape). @@ -43,8 +50,17 @@ function byX(rects: [Shape, Shape]): { left: Shape; right: Shape } { return a.x <= b.x ? { left: a, right: b } : { left: b, right: a }; } -/** Font ids already handed out by `unloadedFont`. */ -const claimedFonts = new Set(); +/** Tracks fonts used by this test run. */ +const claimedFontsByRun = new WeakMap>(); + +function claimedFonts(ctx: TestContext): Set { + let claimed = claimedFontsByRun.get(ctx.penpot); + if (!claimed) { + claimed = new Set(); + claimedFontsByRun.set(ctx.penpot, claimed); + } + return claimed; +} /** * Picks an unclaimed font differing from the text's current one, so assigning @@ -53,11 +69,12 @@ const claimedFonts = new Set(); */ function unloadedFont(ctx: TestContext, t: Text): Font { const all = ctx.penpot.fonts.all; + const claimed = claimedFonts(ctx); for (let i = all.length - 1; i >= 0; i--) { const f = all[i]; if (f.fontId === t.fontId || f.variants.length === 0) continue; - if (claimedFonts.has(f.fontId)) continue; - claimedFonts.add(f.fontId); + if (claimed.has(f.fontId)) continue; + claimed.add(f.fontId); return f; } throw new Error('no alternative font available'); @@ -351,6 +368,89 @@ describe('WaitForLayoutUpdate', () => { }); }); + describe('Components', () => { + test('wait covers propagation from a component main to its copy', async (ctx) => { + const source = ctx.penpot.createRectangle(); + ctx.board.appendChild(source); + const component = ctx.penpot.library.local.createComponent([source]); + const main = component.mainInstance() as Board; + const copy = component.instance() as Board; + ctx.board.appendChild(copy); + await ctx.penpot.waitForLayoutUpdate(); + + const mainChild = main.children[0]; + const copyChild = copy.children[0]; + mainChild.opacity = 0.37; + + await ctx.penpot.waitForLayoutUpdate(); + expect(copyChild.opacity).toBeCloseTo(0.37); + }); + + test('shape wait covers propagation from a component main to its copy', async (ctx) => { + const source = ctx.penpot.createRectangle(); + ctx.board.appendChild(source); + const component = ctx.penpot.library.local.createComponent([source]); + const main = component.mainInstance() as Board; + const copy = component.instance() as Board; + ctx.board.appendChild(copy); + await ctx.penpot.waitForLayoutUpdate(); + + const mainChild = main.children[0]; + const copyChild = copy.children[0]; + mainChild.opacity = 0.63; + + await copyChild.waitForLayoutUpdate(); + expect(copyChild.opacity).toBeCloseTo(0.63); + }); + + test('wait covers layout triggered by component propagation', async (ctx) => { + const host = flexBoard(ctx); + const flex = host.addFlexLayout(); + flex.dir = 'row'; + flex.columnGap = 10; + const first = ctx.penpot.createRectangle(); + first.resize(50, 50); + flex.appendChild(first); + const second = ctx.penpot.createRectangle(); + second.resize(50, 50); + flex.appendChild(second); + + const component = ctx.penpot.library.local.createComponent([host]); + const main = component.mainInstance() as Board; + const copy = component.instance() as Board; + ctx.board.appendChild(copy); + await ctx.penpot.waitForLayoutUpdate(); + + const { left: mainLeft } = byX([main.children[0], main.children[1]]); + mainLeft.resize(120, 50); + + await ctx.penpot.waitForLayoutUpdate(); + const { left: copyLeft, right: copyRight } = byX([ + copy.children[0], + copy.children[1], + ]); + expect(copyLeft.width).toBeCloseTo(120, 0); + expect(copyRight.x - copyLeft.x).toBeCloseTo(130, 0); + }); + }); + + describe('Library assets', () => { + test('wait covers propagation from a local color to a referenced shape', async (ctx) => { + const color = ctx.penpot.library.local.createColor(); + color.color = '#112233'; + color.opacity = 1; + const rect = ctx.penpot.createRectangle(); + rect.fills = [color.asFill()]; + ctx.board.appendChild(rect); + await ctx.penpot.waitForLayoutUpdate(); + + color.color = '#aabbcc'; + + await ctx.penpot.waitForLayoutUpdate(); + expect(rect.fills[0]?.fillColor).toBe('#aabbcc'); + }); + }); + // A font applied to a group reaches its text descendants, so the work is // pending on the children and never on the group itself. Skipped under a // mocked backend for the same reason as the Text group: no fonts are served, diff --git a/plugins/apps/plugin-api-test-suite/src/ui.css b/plugins/apps/plugin-api-test-suite/src/ui.css index 104ac64c90..6e9913f1e4 100644 --- a/plugins/apps/plugin-api-test-suite/src/ui.css +++ b/plugins/apps/plugin-api-test-suite/src/ui.css @@ -60,15 +60,41 @@ body { background-color: var(--background-secondary); } -.group-summary { +.group-header { display: flex; align-items: center; gap: var(--spacing-8, 8px); padding: var(--spacing-8, 8px); +} + +/* Makes the full header row toggle the group. */ +.group-toggle { + display: flex; + flex: 1; + align-items: center; + gap: var(--spacing-8, 8px); + min-width: 0; + margin: 0; + padding: 0; + border: none; + background: none; + color: inherit; + font: inherit; + text-align: start; cursor: pointer; user-select: none; } +.group-chevron { + flex: 0 0 auto; + color: var(--foreground-secondary); + transition: transform 0.15s ease; +} + +.group-toggle[aria-expanded='true'] .group-chevron { + transform: rotate(90deg); +} + .group-name { color: var(--foreground-primary); } @@ -144,6 +170,11 @@ body { padding: 0; } +/* Keeps hidden test lists out of the layout. */ +.test-list[hidden] { + display: none; +} + .test-row { display: grid; grid-template-columns: 1fr auto auto; diff --git a/plugins/apps/plugin-api-test-suite/src/ui.ts b/plugins/apps/plugin-api-test-suite/src/ui.ts index 4aacfd1a8e..fe5fc8a7cf 100644 --- a/plugins/apps/plugin-api-test-suite/src/ui.ts +++ b/plugins/apps/plugin-api-test-suite/src/ui.ts @@ -128,6 +128,13 @@ function reloadIcon(): SVGSVGElement { return svgIcon(['M13 8a5 5 0 1 1-1.46-3.54', 'M13 2.5v3h-3'], false); } +/** Shows whether a group is expanded. */ +function chevronIcon(): SVGSVGElement { + const icon = svgIcon(['M6 3.5 10.5 8 6 12.5'], false); + icon.classList.add('group-chevron'); + return icon; +} + function render() { root.replaceChildren( renderHeader(), @@ -273,9 +280,12 @@ function renderRow(test: TestMeta): HTMLElement { return row; } -function renderGroupSummary( +/** Builds a group header with separate select, toggle, and run controls. */ +function renderGroupHeader( name: string, groupTestList: TestMeta[], + panelId: string, + expanded: boolean, ): HTMLElement { const statuses = groupTestList.map( (t) => results.get(t.id)?.status ?? 'pending', @@ -291,12 +301,12 @@ function renderGroupSummary( const groupCheckbox = el('input', { type: 'checkbox', className: 'checkbox-input', + title: `Select every test in "${name}"`, + ariaLabel: `Select every test in "${name}"`, checked: selectedCount === total && total > 0, disabled: running, }); groupCheckbox.indeterminate = selectedCount > 0 && selectedCount < total; - // Keep the checkbox from toggling the
when clicked. - groupCheckbox.addEventListener('click', (e) => e.stopPropagation()); groupCheckbox.addEventListener('change', () => { if (groupCheckbox.checked) ids.forEach((id) => selected.add(id)); else ids.forEach((id) => selected.delete(id)); @@ -305,17 +315,14 @@ function renderGroupSummary( const runButton = el('button', { className: 'icon-button run-group', + type: 'button', title: `Run "${name}"`, ariaLabel: `Run "${name}"`, disabled: running, }); runButton.dataset.appearance = 'secondary'; runButton.append(playIcon()); - runButton.addEventListener('click', (e) => { - e.preventDefault(); - e.stopPropagation(); - run(ids); - }); + runButton.addEventListener('click', () => run(ids)); const counts = el('span', { className: 'group-counts' }, [ el('span', { className: 'count-pass', textContent: `${passed}` }), @@ -327,14 +334,26 @@ function renderGroupSummary( }), ]); - return el('summary', { className: 'group-summary' }, [ - groupCheckbox, + const toggle = el('button', { className: 'group-toggle', type: 'button' }, [ + chevronIcon(), el('span', { className: `status-dot dot-${aggregate}`, title: statusLabel(aggregate), }), el('span', { className: 'group-name', textContent: name }), counts, + ]); + toggle.setAttribute('aria-expanded', String(expanded)); + toggle.setAttribute('aria-controls', panelId); + toggle.addEventListener('click', () => { + if (expanded) expandedGroups.delete(name); + else expandedGroups.add(name); + render(); + }); + + return el('div', { className: 'group-header' }, [ + groupCheckbox, + toggle, runButton, ]); } @@ -342,25 +361,24 @@ function renderGroupSummary( function renderList(): HTMLElement { const container = el('div', { className: 'groups' }); - for (const group of groupTests()) { - const details = el('details', { className: 'group' }); + groupTests().forEach((group, index) => { // Groups are collapsed by default; remember the ones the user expands. - details.open = expandedGroups.has(group.name); - details.addEventListener('toggle', () => { - if (details.open) expandedGroups.add(group.name); - else expandedGroups.delete(group.name); - }); + const expanded = expandedGroups.has(group.name); + const panelId = `group-panel-${index}`; - details.append(renderGroupSummary(group.name, group.tests)); - - const list = el('ul', { className: 'test-list' }); + const list = el('ul', { className: 'test-list', id: panelId }); + list.hidden = !expanded; for (const test of group.tests) { list.append(renderRow(test)); } - details.append(list); - container.append(details); - } + container.append( + el('div', { className: 'group' }, [ + renderGroupHeader(group.name, group.tests, panelId, expanded), + list, + ]), + ); + }); return container; } diff --git a/plugins/libs/plugin-types/index.d.ts b/plugins/libs/plugin-types/index.d.ts index 483cfecca8..abb4e981c1 100644 --- a/plugins/libs/plugin-types/index.d.ts +++ b/plugins/libs/plugin-types/index.d.ts @@ -1353,12 +1353,13 @@ export interface Context { /** * This method returns a promise that will be resolved when all the - * pending layout updates have finished. If no layout work is pending - * the promise resolves immediately. + * pending layout updates have finished and the components have synchronized. + * If no layout work is pending the promise resolves immediately. * @param timeout Maximum time to wait, in milliseconds. If the timeout * elapses before the layout settles, the promise is rejected. Defaults to * 30000; the promise never waits indefinitely. - * @return The promise to be resolved when the layout is updated + * @return The promise to be resolved when the layout is updated. It is + * rejected with an Error, both on timeout and on an invalid timeout value. */ waitForLayoutUpdate(timeout?: number): Promise; } @@ -4109,13 +4110,14 @@ export interface ShapeBase extends PluginData { remove(): void; /** - * This method returns a promise that will be resolved when the pending - * layout updates for this shape and its children have finished. If no layout - * work is pending for them the promise resolves immediately. + * This method returns a promise that will be resolved when all the + * pending layout updates have finished and the components have synchronized. + * If no layout work is pending the promise resolves immediately. * @param timeout Maximum time to wait, in milliseconds. If the timeout * elapses before the shape's layout settles, the promise is rejected. * Defaults to 30000; the promise never waits indefinitely. - * @return The promise to be resolved when the shape's layout is updated + * @return The promise to be resolved when the shape's layout is updated. It + * is rejected with an Error, both on timeout and on an invalid timeout value. */ waitForLayoutUpdate(timeout?: number): Promise; } From 7061ecae0a1ae38488adfdecf32e7d19e6b497ea Mon Sep 17 00:00:00 2001 From: Luis de Dios Date: Tue, 18 Aug 2026 17:50:15 +0200 Subject: [PATCH 07/19] :bug: Fix gitch of placeholder when switching between teams on dashboard (#10922) * :bug: Fix use single point for retrieving state and propagate it * :bug: Fix use loading message instead of placeholder when loading files --- frontend/src/app/main/ui/dashboard.cljs | 84 +++++++++++-------- .../src/app/main/ui/dashboard/deleted.cljs | 12 +-- frontend/src/app/main/ui/dashboard/files.cljs | 20 ++--- .../src/app/main/ui/dashboard/projects.cljs | 16 ++-- 4 files changed, 64 insertions(+), 68 deletions(-) diff --git a/frontend/src/app/main/ui/dashboard.cljs b/frontend/src/app/main/ui/dashboard.cljs index 622873ac4c..17c6cfe69a 100644 --- a/frontend/src/app/main/ui/dashboard.cljs +++ b/frontend/src/app/main/ui/dashboard.cljs @@ -27,6 +27,7 @@ [app.main.ui.dashboard.files :refer [files-section*]] [app.main.ui.dashboard.fonts :refer [fonts-page* font-providers-page*]] [app.main.ui.dashboard.import] + [app.main.ui.dashboard.layout-toggle :as lt] [app.main.ui.dashboard.libraries :refer [libraries-page*]] [app.main.ui.dashboard.projects :refer [projects-section*]] [app.main.ui.dashboard.search :refer [search-page*]] @@ -51,7 +52,7 @@ (mf/defc dashboard-content* {::mf/private true} - [{:keys [team projects project section search-term profile default-project]}] + [{:keys [team projects project section search-term profile default-project layout on-layout-change]}] (let [container (mf/use-ref) content-width (mf/use-state 0) @@ -100,18 +101,18 @@ :dashboard-recent (when (seq projects) [:* - [:> projects-section* - {:team team - :projects projects - :profile profile}] + [:> projects-section* {:team team + :projects projects + :profile profile + :layout layout + :on-layout-change on-layout-change}] (when ^boolean show-templates? - [:> templates-section* - {:profile profile - :project-id project-id - :team-id team-id - :default-project-id default-project-id - :content-width @content-width}])]) + [:> templates-section* {:profile profile + :project-id project-id + :team-id team-id + :default-project-id default-project-id + :content-width @content-width}])]) :dashboard-fonts [:> fonts-page* {:team team}] @@ -123,14 +124,15 @@ (when project [:* [:> files-section* {:team team - :project project}] + :project project + :layout layout + :on-layout-change on-layout-change}] (when ^boolean show-templates? - [:> templates-section* - {:profile profile - :team-id team-id - :project-id project-id - :default-project-id default-project-id - :content-width @content-width}])]) + [:> templates-section* {:profile profile + :team-id team-id + :project-id project-id + :default-project-id default-project-id + :content-width @content-width}])]) :dashboard-search [:> search-page* {:team team @@ -155,7 +157,9 @@ :dashboard-deleted [:> deleted-section* {:team team :projects projects - :profile profile}] + :profile profile + :layout layout + :on-layout-change on-layout-change}] nil)])) @@ -313,7 +317,15 @@ (mf/with-memo [projects] (->> projects (filter :is-default) - (first)))] + (first))) + + layout* (hooks/use-persisted-state lt/layout-key lt/default-layout) + layout (deref layout*) + + on-layout-change + (mf/use-fn + (fn [value] + (reset! layout* (keyword value))))] (hooks/use-shortcuts ::dashboard sc/shortcuts-dashboard :dashboard) @@ -347,22 +359,22 @@ ;; team is already set so don't put the team into mf/deps. [:main {:class (stl/css :dashboard) :key (dm/str (:id team))} - [:> sidebar* - {:team team - :projects projects - :project project - :default-project default-project - :profile profile - :section section - :search-term search-term}] - [:> dashboard-content* - {:projects projects - :profile profile - :project project - :default-project default-project - :section section - :search-term search-term - :team team}]]])) + [:> sidebar* {:team team + :projects projects + :project project + :default-project default-project + :profile profile + :section section + :search-term search-term}] + [:> dashboard-content* {:projects projects + :profile profile + :project project + :default-project default-project + :section section + :search-term search-term + :team team + :layout layout + :on-layout-change on-layout-change}]]])) (mf/defc dashboard-page* {::mf/lazy-load true} diff --git a/frontend/src/app/main/ui/dashboard/deleted.cljs b/frontend/src/app/main/ui/dashboard/deleted.cljs index 9a46b697ba..a66714209d 100644 --- a/frontend/src/app/main/ui/dashboard/deleted.cljs +++ b/frontend/src/app/main/ui/dashboard/deleted.cljs @@ -219,16 +219,8 @@ (tr "labels.deleted")]]])) (mf/defc deleted-section* - [{:keys [team projects]}] - (let [layout* (hooks/use-persisted-state lt/layout-key lt/default-layout) - layout (deref layout*) - - on-layout-change - (mf/use-fn - (fn [value] - (reset! layout* (keyword value)))) - - deleted-map + [{:keys [team projects layout on-layout-change]}] + (let [deleted-map (mf/deref ref:deleted-files) projects diff --git a/frontend/src/app/main/ui/dashboard/files.cljs b/frontend/src/app/main/ui/dashboard/files.cljs index ed3781faba..cf2b4fce35 100644 --- a/frontend/src/app/main/ui/dashboard/files.cljs +++ b/frontend/src/app/main/ui/dashboard/files.cljs @@ -137,7 +137,7 @@ :on-import on-import}])]])) (mf/defc files-section* - [{:keys [project team]}] + [{:keys [project team layout on-layout-change]}] (let [files (mf/deref refs/files) project-id (get project :id) @@ -147,7 +147,6 @@ (sort-by :modified-at) (reverse))) - can-edit? (-> team :permissions :can-edit) project-id (:id project) is-draft-proyect (:is-default project) @@ -155,19 +154,16 @@ [rowref limit] (hooks/use-dynamic-grid-item-width) file-count (or (count files) 0) + + loading? (and (some? (:count project)) + (not= (:count project) file-count)) + empty-state-viewer (and (not can-edit?) - (= 0 file-count)) + (= 0 file-count) + (not loading?)) selected-files (mf/deref refs/selected-files) - layout* (hooks/use-persisted-state lt/layout-key lt/default-layout) - layout (deref layout*) - - on-layout-change - (mf/use-fn - (fn [value] - (reset! layout* (keyword value)))) - on-file-created (mf/use-fn (fn [file-data] @@ -216,7 +212,7 @@ (tr "dashboard.empty-placeholder-drafts-subtitle") (tr "dashboard.empty-placeholder-files-subtitle"))}] [:> grid* {:project project - :files files + :files (if loading? nil files) :selected-files selected-files :can-edit can-edit? :origin :files diff --git a/frontend/src/app/main/ui/dashboard/projects.cljs b/frontend/src/app/main/ui/dashboard/projects.cljs index 542395caa7..441bf5cc5c 100644 --- a/frontend/src/app/main/ui/dashboard/projects.cljs +++ b/frontend/src/app/main/ui/dashboard/projects.cljs @@ -109,6 +109,10 @@ team-id (get team :id) file-count (or (:count project) 0) + + loading? (and (pos? (:count project)) + (empty? files)) + is-draft? (:is-default project) empty? (and (not can-edit) (= 0 file-count)) @@ -292,7 +296,7 @@ [:> line-grid* {:project project :team team - :files files + :files (if loading? nil files) :create-fn create-file :can-edit can-edit :limit limit @@ -313,7 +317,7 @@ (l/derived :recent-files st/state)) (mf/defc projects-section* - [{:keys [team projects profile]}] + [{:keys [team projects profile layout on-layout-change]}] (let [team-id (get team :id) @@ -334,14 +338,6 @@ show-deleted? (:can-edit permisions) - layout* (hooks/use-persisted-state lt/layout-key lt/default-layout) - layout (deref layout*) - - on-layout-change - (mf/use-fn - (fn [value] - (reset! layout* (keyword value)))) - projects (mf/with-memo [projects] (->> projects From df664fe96b5b0937bfc1a8de328f2c12d075e286 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 18:05:17 +0200 Subject: [PATCH 08/19] :paperclip: Add improvement for review command --- .opencode/commands/review.md | 107 +++++++-- .opencode/skills/plan-review/SKILL.md | 315 ++++++++++++++++++++++++++ 2 files changed, 398 insertions(+), 24 deletions(-) create mode 100644 .opencode/skills/plan-review/SKILL.md diff --git a/.opencode/commands/review.md b/.opencode/commands/review.md index 3e6f59cee5..23d7941ce7 100644 --- a/.opencode/commands/review.md +++ b/.opencode/commands/review.md @@ -1,27 +1,30 @@ -Act as a senior software engineer and perform a thorough code review. +Act as a senior software engineer and perform a thorough review. ## Instructions +1. **Determine what is being reviewed** from the provided context: + - **If it is a plan** (implementation plan, design document, task breakdown) → follow the **Plan Review** path below. + - **If it is code** (diff, PR, code change) → follow the **Code Review** path below. + +--- + +## Code Review Path + 1. Load the **`code-review-and-quality`** skill — it defines the five axes, core principles (DRY, KISS, YAGNI), severity taxonomy, and output format. -2. Determine the diff or code to review from the provided context. -3. **Skip generated files, lockfile-only changes, and unrelated modifications** unless they introduce security risks. -4. Read the diff and the surrounding context for each changed file. -5. Review across all five axes: correctness, readability, architecture, security, performance. -6. Produce the review using this structure: - - **Summary**: One-paragraph overview of the change and its impact - - **Critical/High Findings**: Blockers that must be fixed (with file:line, severity, description, and proposed fix) - - **Other Findings**: Medium/Low issues and suggestions - - **Testing Recommendations**: Missing test coverage or test quality issues - - **Positive Observations**: What was done well (brief, specific) - - **Verdict**: Approve / Request Changes / Needs Discussion -7. For each finding: +2. Read `AGENTS.md` and follow its instructions for finding and reading all related testing documentation from memories before reviewing the code. +3. Determine the diff or code to review from the provided context. +4. **Skip generated files, lockfile-only changes, and unrelated modifications** unless they introduce security risks. +5. Read the diff and the surrounding context for each changed file. +6. Review across all five axes: correctness, readability, architecture, security, performance. +7. Produce the review using the **Code Review Format** below. +8. For each finding: - State the severity (Critical / High / Medium / Low / Suggestion) - Identify the file and line - Describe failure circumstances - **For Critical/High**: Provide a concrete fix with a code snippet showing the corrected code - **For Medium/Low**: Describe the fix clearly; code snippet optional - If multiple approaches exist, briefly note trade-offs -8. **Perform a second review pass if the change is complex:** +9. **Perform a second review pass if the change is complex:** - **Complex indicators**: Critical/High findings, multiple files (>5), architectural changes, security-sensitive code, >300 lines changed - **Skip for simple changes**: Typo fixes, formatting, small bug fixes (<50 lines), single-file changes with no findings - Second pass checks: @@ -30,45 +33,76 @@ Act as a senior software engineer and perform a thorough code review. - Remove false positives: Discard findings that aren't real issues - Verify fixes: Are the proposed solutions actually correct and complete? +--- + +## Plan Review Path + +1. Load the **`plan-review`** skill — it defines the six axes, severity taxonomy, and output format. +2. Read the full plan from the provided context. +3. Review across all six axes: completeness, task quality, architecture & sequencing, risk coverage, actionability, and proposed code quality (if the plan includes implementation details). +4. Produce the review using the **Plan Review Format** below. +5. For each finding: + - State the severity (Critical / Required / Nit / Optional / FYI) + - Identify the section or task it refers to + - Describe the gap or problem + - **For Critical/Required**: Propose a concrete fix or addition + - **For Nit/Optional**: Describe the improvement; concrete text optional +6. **Perform a second review pass if the plan is complex:** + - **Complex indicators**: Critical findings, >10 tasks, migrations or breaking changes, security-sensitive features + - **Skip for simple plans**: 1–2 tasks, no risks, no code proposals + - Second pass checks: + - Validate severity assignments + - Catch missed gaps: edge cases, missing dependencies, unaddressed risks + - Remove false positives + - Verify proposed remedies are actionable + +--- + ## Strong Rules 1. Do not invent problems. Every finding must be real and actionable. 2. Do not modify any code and do not create a commit — this command only reviews. 3. Be specific and constructive. "This could be better" is not helpful — explain why and how. 4. Prioritize by impact. One structural issue outweighs ten nits. -5. If tests are missing for new functionality, flag it as High severity. +5. Missing tests are an issue, not a suggestion. If tests are missing or inadequate for new functionality, report it as a severity-tagged finding in the findings sections below — High severity (code) or Required (plan) — never as a recommendation. ## Context $ARGUMENTS -## Expected Format +## Expected Format — Code Review ``` ## Review Summary [1-2 sentences on what the change does and overall assessment] ## Critical/High Findings + ### [Severity] file.ts:123 **Issue**: [Description of the problem] -**Impact**: [What could go wrong] +**Impact**: [What could go wrong if this is not fixed] **Fix**: -```[language] + +````[language] // Current code [problematic code] // Fixed code [corrected code] -``` [Optional: note trade-offs if multiple approaches exist] +```` + +### [Severity] file.ts:456 +**Issue**: [Description of the problem] +**Impact**: [What could go wrong if this is not fixed] +**Fix**: [Clear description of the fix; code snippet if it clarifies] ## Other Findings -### [Severity] file.ts:456 -**Issue**: [Description] -**Fix**: [Clear description; code snippet optional] -## Testing Recommendations -[List specific test cases that should be added] +### [Severity] file.ts:789 +**Issue**: [Description] +**Impact**: [Minor consequence or risk] +**Fix**: [Clear description; code snippet optional] ## Positive Observations [2-3 specific things done well] @@ -77,3 +111,28 @@ $ARGUMENTS [Approve / Request Changes / Needs Discussion] [If Request Changes: list the must-fix items] ``` + +## Expected Format — Plan Review + +``` +## Review Summary +[1-2 sentences on the plan's goal and overall assessment] + +## Critical/Required Findings +### [Severity] [Section or Task N] +**Issue**: [Description of the gap or problem] +**Impact**: [What could go wrong during implementation] +**Proposed fix**: [Concrete addition or change to the plan] + +## Other Findings +### [Severity] [Section or Task N] +**Issue**: [Description] +**Proposed fix**: [Clear description; concrete text optional] + +## Strengths +[2-3 specific things done well in the plan] + +## Verdict +[Approve / Request Changes / Needs Discussion] +[If Request Changes: list the must-fix items] +``` diff --git a/.opencode/skills/plan-review/SKILL.md b/.opencode/skills/plan-review/SKILL.md new file mode 100644 index 0000000000..60b61c75e8 --- /dev/null +++ b/.opencode/skills/plan-review/SKILL.md @@ -0,0 +1,315 @@ +--- +name: plan-review +description: Reviews implementation plans for quality, completeness, and actionability. Use after a plan is produced by the planner skill, before starting implementation. Use when evaluating a plan written by yourself, another agent, or a human. +--- + +# Plan Review + +## Overview + +Multi-dimensional plan review with quality gates. Every plan gets reviewed before implementation starts — no exceptions. Review covers six axes: completeness, task quality, architecture & sequencing, risk coverage, actionability, and proposed code quality. + +**The approval standard:** Approve a plan when it is specific enough that a skilled implementer could execute it without guessing, the task ordering is sound, and risks are acknowledged. Perfect plans don't exist — the goal is confidence that implementation won't derail. Don't block a plan because it isn't exactly how you would have structured it. If it's executable and well-organized, approve it. + +## When to Use + +- After the planner skill produces a plan +- Before starting implementation on any non-trivial task +- When reviewing a plan written by another agent or a human +- When a plan feels too large, vague, or risky to start + +**Do NOT use for:** Single-file changes with obvious scope, or when the task is trivial enough to just do. + +## The Six-Axis Review + +Every plan gets evaluated across these dimensions: + +### 1. Completeness + +Does the plan cover everything needed to implement successfully? + +- Is the **context** clear? (What problem, why now, what's the goal?) +- Are **affected modules** identified with paths? +- Are **architecture decisions** documented with rationale? +- Is there a **testing strategy**? +- Are **verification commands** explicit (not "run the tests")? +- Are **open questions** listed (not buried in someone's head)? +- Is there a **parallelization** assessment for multi-task plans? + +**Missing any of these is a gap, not a nit.** + +### 2. Task Quality + +Are the tasks well-defined and independently executable? + +- Does every task have **acceptance criteria**? (Testable, not vague) +- Does every task have **verification steps**? +- Are tasks **sized appropriately**? (XS–M is ideal, L is acceptable, XL must be split) +- Are **dependencies** between tasks explicitly stated? +- Are **files likely touched** listed? +- Is each task a **single, self-contained change**? (Not "implement the whole feature") +- Could a skilled implementer pick up any task and execute it without asking clarifying questions? + +### 3. Architecture & Sequencing + +Is the plan structured so implementation flows correctly? + +- Does implementation order follow the **dependency graph** (foundations first)? +- Are tasks **vertically sliced** (feature paths) rather than horizontally layered? +- Does each task leave the system in a **working state**? +- Are there **checkpoints** between major phases? +- Are **high-risk tasks early** (fail fast)? +- Is the total plan a reasonable number of tasks? (More than ~15 tasks suggests the scope should be split into multiple plans) + +### 4. Risk Coverage + +Are the hard parts acknowledged and mitigated? + +- Are **edge cases** identified? +- Are **breaking changes** or **migration concerns** noted? +- Are **security implications** considered? +- Are **performance implications** considered? +- Are **external dependencies** or integration risks flagged? +- Is there a plan for **rollback** if something goes wrong? +- Are **data integrity** risks addressed (what happens if a migration fails mid-way)? + +### 5. Actionability + +Can an implementer actually execute this? + +- Are **file paths** specific (not "update the relevant files")? +- Are **function/method names** mentioned where applicable? +- Are **verification commands** copy-pasteable (not "run the linter")? +- Are **test commands** project-specific (not generic)? +- Is the **code shape** described where the implementation isn't obvious? +- Are **conventions** referenced (naming, patterns, existing utilities to reuse)? +- Does the plan reference **existing code** the implementer should read first? + +### 6. Proposed Code Quality *(when the plan includes implementation details)* + +If the plan proposes code shapes, function signatures, data structures, or API designs, evaluate those proposals against `code-review-and-quality` criteria: + +- **Correctness:** Do the proposed types/signatures handle edge cases (null, empty, boundaries)? +- **Readability:** Are proposed names descriptive and consistent with project conventions? +- **Architecture:** Do proposed abstractions follow existing patterns? Are they justified (not over-engineered)? +- **Security:** Do proposed APIs validate input at boundaries? Any injection/XSS vectors in the design? +- **Performance:** Do proposed data structures avoid N+1 patterns? Any unbounded operations in the design? + +**When to apply:** Only when the plan includes specific code snippets, type definitions, API contracts, or function signatures. Plans that only describe "what" without showing "how" skip this axis. + +## Structural Remedies + +When you flag a structural problem in a plan, propose the fix — not just the problem: + +- **A task is too large (XL):** Split it into vertical slices. Each slice should be independently testable. +- **Missing acceptance criteria:** Draft 2–3 specific, testable conditions for the task. +- **Wrong sequencing:** Identify the dependency and propose the correct order. +- **No checkpoints:** Suggest where checkpoints should go (typically after every 2–3 tasks). +- **Vague verification:** Replace "run tests" with the actual project command. +- **Horizontal slicing:** Restructure into vertical feature paths. +- **Missing risk section:** Draft the risks you can identify from the plan content. + +Prefer the remedy that makes the plan immediately actionable over one that just flags the gap. + +## Plan Sizing + +Plans should be scoped to a single deliverable: + +``` +1–5 tasks → Good. A focused feature or bug fix. +6–10 tasks → Acceptable for a moderate feature. +11–15 tasks → Large. Consider splitting into phases. +15+ tasks → Too large. Split into multiple plans. +``` + +**What counts as "one plan":** A self-contained set of changes that delivers a single coherent capability. If you can describe the goal in one sentence, it's one plan. + +## Categorize Findings + +Label every comment with its severity so the author knows what's required vs optional: + +| Prefix | Meaning | Author Action | +|--------|---------|---------------| +| *(no prefix)* | Required change | Must address before implementation starts | +| **Critical:** | Blocks implementation | Missing security consideration, data integrity risk, fundamentally wrong approach | +| **Nit:** | Minor, optional | Author may ignore — wording, formatting | +| **Optional:** / **Consider:** | Suggestion | Worth considering but not required | +| **FYI** | Informational only | No action needed — context for future reference | + +**Lead with what matters.** Order findings by leverage: missing risks and wrong sequencing first, then task quality gaps, then completeness, then nits. If you have one critical sequencing problem and ten nits, the sequencing problem *is* the review. + +## Review Process + +### Step 1: Understand the Goal + +Before evaluating structure, understand intent: + +``` +- What is this plan trying to accomplish? +- What problem does it solve? +- What does "done" look like? +``` + +### Step 2: Check Completeness First + +Scan for missing sections before diving into content: + +``` +- Context present? +- Affected modules listed? +- Architecture decisions documented? +- Risks acknowledged? +- Testing strategy defined? +- Verification commands explicit? +``` + +### Step 3: Review Task Quality + +Walk through each task: + +``` +For each task: +1. Can I tell exactly what to build? +2. Are acceptance criteria specific and testable? +3. Is the size reasonable (not XL)? +4. Are dependencies clear? +5. Would I know which files to touch? +``` + +### Step 4: Validate Sequencing + +Check the dependency graph: + +``` +- Are foundations built first? +- Does each task leave the system working? +- Are checkpoints placed correctly? +- Are high-risk items early? +- Is it vertically sliced? +``` + +### Step 5: Assess Actionability + +Put yourself in the implementer's shoes: + +``` +- Could I pick up task 1 and start coding without asking any questions? +- Are the verification commands copy-pasteable? +- Are file paths and function names specific? +- Is existing code referenced where I'd need to read it? +``` + +### Step 6: Verify the Verification Story + +Check that the plan can actually confirm it worked: + +``` +- What tests should pass after implementation? +- What build/compile commands are relevant? +- What manual checks are needed? +- How do we know the feature works end-to-end? +``` + +### Step 7: Evaluate Proposed Code Quality *(if applicable)* + +If the plan includes code snippets, types, or API designs: + +``` +- Load code-review-and-quality skill for criteria +- Check proposed signatures for edge cases +- Verify naming follows project conventions +- Confirm abstractions follow existing patterns +- Scan for security vectors in proposed APIs +- Check for performance issues in proposed data structures +``` + +## Review Checklist + +```markdown +## Review: [Plan title] + +### Completeness +- [ ] Context explains the problem and goal +- [ ] Affected modules are listed with paths +- [ ] Architecture decisions have rationale +- [ ] Testing strategy is defined +- [ ] Verification commands are explicit and project-specific +- [ ] Open questions are listed + +### Task Quality +- [ ] Every task has acceptance criteria +- [ ] Every task has verification steps +- [ ] Tasks are sized XS–M (L acceptable, XL must be split) +- [ ] Task dependencies are stated +- [ ] Files likely touched are listed + +### Architecture & Sequencing +- [ ] Order follows dependency graph (foundations first) +- [ ] Vertically sliced (not horizontal layers) +- [ ] Each task leaves system working +- [ ] Checkpoints exist between phases +- [ ] High-risk tasks are early + +### Risk Coverage +- [ ] Edge cases identified +- [ ] Breaking changes / migrations noted +- [ ] Security implications considered +- [ ] Performance implications considered +- [ ] Rollback strategy exists (if applicable) + +### Actionability +- [ ] File paths are specific +- [ ] Verification commands are copy-pasteable +- [ ] Existing code to read is referenced +- [ ] Conventions and patterns are noted + +### Proposed Code Quality *(if plan includes implementation details)* +- [ ] Proposed types/signatures handle edge cases +- [ ] Proposed names follow project conventions +- [ ] Proposed abstractions follow existing patterns +- [ ] No security vectors in proposed APIs +- [ ] No performance issues in proposed structures + +### Verdict +- [ ] **Approve** — Ready to implement +- [ ] **Request changes** — Gaps must be addressed +``` + +## Common Rationalizations + +| Rationalization | Reality | +|---|---| +| "I'll figure out the details during implementation" | That's how you discover blocking dependencies mid-task. Surface them now. | +| "The tasks are obvious, no need for criteria" | Write them anyway. Explicit criteria surface hidden assumptions. | +| "It's just a small feature, it doesn't need a plan" | Small features have edge cases too. 3 tasks with criteria takes 5 minutes. | +| "The plan is good enough" | "Good enough" without acceptance criteria means the implementer defines "done" — and they might define it differently. | +| "I'll add verification steps later" | Later never comes. The plan is the contract — define verification now. | +| "Risks are minimal" | Every change has risks. If you can't name them, you haven't thought about them. | +| "The file paths are obvious" | They're obvious to the author. The implementer might not know the codebase. | +| "The code in the plan is fine, it'll get reviewed later" | Plan-level code review catches design problems before implementation — fixing them after coding is more expensive. | + +## Red Flags + +- No acceptance criteria on any task +- Tasks that say "implement the feature" without specifics +- No verification steps anywhere in the plan +- All tasks are XL-sized +- No checkpoints between phases +- Dependency order isn't considered (e.g., API handler before domain model) +- No testing strategy +- Verification commands are generic ("run tests") instead of project-specific +- Plan has 20+ tasks (scope too large for one plan) +- No risk section on a plan with migrations, breaking changes, or security implications +- Horizontal slicing (all domain, then all services, then all API) +- File paths are vague ("update the relevant files") +- Missing open questions section despite stated unknowns +- Proposed code ignores project conventions or existing patterns +- Proposed types use gratuitous `any`/`unknown`/optional without justification +- Proposed APIs don't validate input at boundaries + +## See Also + +- For producing plans, use the `planner` skill +- For reviewing implemented code, use `code-review-and-quality` — also the criteria source for axis 6 +- For security-specific concerns, see `security-and-hardening` +- For testing strategy guidance, see `testing` From 4339d8d244c193b1c75903b1803ca4d41c543e41 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 18 Aug 2026 18:37:58 +0200 Subject: [PATCH 09/19] :paperclip: Update serena documentation about backend storage --- .serena/memories/backend/core.md | 4 +- .../http-storage-filedata-subtleties.md | 7 +- .serena/memories/backend/storage.md | 83 +++++++++++++++++++ .serena/memories/prod-infra/core.md | 4 +- 4 files changed, 89 insertions(+), 9 deletions(-) create mode 100644 .serena/memories/backend/storage.md diff --git a/.serena/memories/backend/core.md b/.serena/memories/backend/core.md index 708d30fc5d..7b085856d1 100644 --- a/.serena/memories/backend/core.md +++ b/.serena/memories/backend/core.md @@ -5,7 +5,8 @@ Backend: JVM Clojure; Integrant; PostgreSQL; Redis/Valkey; RPC; HTTP; storage; m ## Focused memories - RPC, DB helpers, workers, cron: `mem:backend/rpc-db-worker-subtleties` -- HTTP sessions, config, storage, media, file data persistence: `mem:backend/http-storage-filedata-subtleties` +- Storage abstraction, logical buckets, object lifecycle, deduplication, access, and garbage collection: `mem:backend/storage`. +- HTTP sessions, config, media processing, and file data persistence: `mem:backend/http-storage-filedata-subtleties`. - Auth flows, permission model, teams, projects, invitations, comments, webhooks, audit: `mem:backend/auth-permissions-product-domains` - Services, task-queue/Pub-Sub topology constraints -> `mem:prod-infra/core`. @@ -107,4 +108,3 @@ IMPORTANT: all CLI commands must be executed from the `backend/` subdirectory. J * **Isolated run:** `clojure -M:dev:test --focus backend-tests.my-ns-test` for a specific test namespace. * **Regression run:** `clojure -M:dev:test` to ensure no regressions in related functional areas. * **Principles:** Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:testing`. - diff --git a/.serena/memories/backend/http-storage-filedata-subtleties.md b/.serena/memories/backend/http-storage-filedata-subtleties.md index e9c0962371..188ece7277 100644 --- a/.serena/memories/backend/http-storage-filedata-subtleties.md +++ b/.serena/memories/backend/http-storage-filedata-subtleties.md @@ -14,10 +14,7 @@ ## Storage and media -- Storage has a fixed valid bucket set. Backends are `:fs` and `:s3`; default backend comes from deprecated `assets-storage-backend` only when present, otherwise `objects-storage-backend`, defaulting to `:fs`. -- `put-object!` creates the DB `storage_object` row before writing backend content. Backend writes happen only for newly created rows, so deduplication can skip object writes. -- Deduplication only applies when requested, when the content can provide a hash, and when bucket metadata is present. Reads exclude soft-deleted storage rows. -- `sto/resolve` can reuse the current DB connection via `::db/reuse-conn true`; preserve this in transaction-sensitive code. +- Storage abstraction, backend configuration, logical buckets, object lifecycle, deduplication, access rules, and garbage collection: `mem:backend/storage`. - SVG validation strips DOCTYPE and uses secure SAX parsing. Basic SVG info falls back to 100x100 dimensions when width/height/viewBox are missing. - Raster metadata is shell-derived with ImageMagick `identify`, verifies detected MIME against the supplied MIME, and swaps dimensions for EXIF orientations 6/8. - Remote image download requires 2xx status, `content-length`, a known MIME, and size under the configured maximum before writing the temp file; mismatched byte count is an internal error. @@ -28,4 +25,4 @@ - File data backends are `legacy-db`, `db`, and `storage`. The storage backend keeps encoded file data in storage bucket `file-data`; the DB row stores metadata with `storage-ref-id` and nil data. - `fdata/upsert!` touches any storage object referenced by incoming metadata before storing the new row/blob. - Pointer-map fragments are persisted separately as type `fragment`, and only modified pointer maps are written. -- `fdata/realize` combines pointer realization and object-map realization. Use it before operations that need complete in-memory file data instead of pointer placeholders. \ No newline at end of file +- `fdata/realize` combines pointer realization and object-map realization. Use it before operations that need complete in-memory file data instead of pointer placeholders. diff --git a/.serena/memories/backend/storage.md b/.serena/memories/backend/storage.md new file mode 100644 index 0000000000..b0d81330f6 --- /dev/null +++ b/.serena/memories/backend/storage.md @@ -0,0 +1,83 @@ +# Backend Storage + +## Abstraction + +- `app.storage` stores binary objects. +- Each object has a `storage_object` database row. +- The row stores the UUID, size, backend, timestamps, and Transit metadata. +- The backend stores the binary content. +- Supported backends are `:fs` and `:s3`. +- FS uses one root directory and a UUID-derived path. +- S3 uses one configured bucket and an optional prefix. +- A Penpot bucket is metadata. It is not an S3 bucket or a filesystem directory. +- FS and S3 use the same UUID-derived object path. The bucket does not change the path. +- `PENPOT_OBJECTS_STORAGE_*` configures the current object backend. +- Deprecated asset-storage config keys remain supported for migration. +- Database rows keep the backend name. Keep the legacy `:assets-fs` and `:assets-s3` aliases. + +## Object Lifecycle + +- `put-object!` creates the database row before it writes backend content. +- Backend content is written only when the row is new. +- A failed backend write can leave an unreferenced database row. +- Callers often set `:touched-at` so garbage collection can remove such rows. +- `get-object` excludes rows with `deleted_at`. +- Existing object values can remain readable until physical deletion. +- `:expired-at` blocks reads after the expiration time. +- `del-object!` sets `deleted_at`. It does not remove backend content. +- `storage-gc-deleted` removes the database row and backend content after the deletion delay. +- `storage-gc-touched` finds references before it sets `deleted_at`. +- `objects-gc` removes deleted domain rows and touches their storage object IDs. +- Use `::db/reuse-conn true` with `sto/resolve` inside a database transaction. + +## Deduplication + +- Deduplication requires `::sto/deduplicate?`, a content hash, and bucket metadata. +- The lookup matches hash, bucket, backend, and `deleted_at IS NULL`. +- The lookup does not include file ID, profile ID, team ID, or organization ID. +- Objects can therefore share content across users and files within one bucket. +- Deleted objects are not reused. +- `tempfile` objects never use deduplication, even when the caller requests it. +- Use `sto/wrap-with-hash` when the caller already calculated the content hash. + +## Bucket Rules + +| Bucket | Content and references | Dedup | Direct `/assets/by-id` access | Cleanup | +| --- | --- | --- | --- | --- | +| `file-media-object` | Original file images and generated media thumbnails. References: `file_media_object.media_id` and `thumbnail_id`. | Yes | Public | Reference scan. | +| `team-font-variant` | Font variants in `team_font_variant`. References: `woff1_file_id`, `woff2_file_id`, `otf_file_id`, and `ttf_file_id`. | Yes | Public | Reference scan. | +| `file-object-thumbnail` | Frame and component thumbnails in `file_tagged_object_thumbnail.media_id`. | Yes | Public | Reference scan. | +| `file-thumbnail` | File grid thumbnails in `file_thumbnail.media_id`. | Yes | Authentication required | Reference scan. | +| `profile` | User and team profile photos. References: `profile.photo_id` and `team.photo_id`. | Yes | Authentication required | Reference scan. | +| `organization` | Organization logos uploaded by the Nitrate management API. | Yes | Public | No reference scan. A touched object is deleted. | +| `tempfile` | Export files, chunked-upload chunks, and temporary font downloads. | No | Authentication required | No reference scan. A touched object uses a two-hour deletion delay. | +| `file-data` | Encoded file data when `file-data-backend` is `storage`. Reference metadata has `storage-ref-id`, `file-id`, and the `file_data` row ID. | Yes | Authentication required | Reference scan. | +| `file-data-fragment` | Compatibility value for file-data fragments. The current backend has no dedicated producer for this bucket. | No current write semantics | Public | No touched-object collector case. | +| `file-change` | Compatibility value for file changes. Current snapshots store data in `file_data`, not this bucket. | No current write semantics | Authentication required | No touched-object collector case. | + +- The valid bucket set lives in `app.storage/valid-buckets`. +- `file-media-object` is the default bucket for old rows without bucket metadata. +- Do not assign a new bucket without adding its access and cleanup behavior. +- The touched-object collector raises an internal error for an unknown bucket. +- It supports `file-media-object`, `team-font-variant`, `file-object-thumbnail`, `file-thumbnail`, `profile`, `file-data`, `tempfile`, and `organization`. +- It does not support `file-data-fragment` or `file-change`. + +## Access Rules + +- `app.http.assets` decides direct object authentication from the bucket. +- Public buckets are `file-media-object`, `file-object-thumbnail`, `team-font-variant`, `file-data-fragment`, and `organization`. +- Other valid buckets require a session or access-token profile ID. +- File-media routes also require file read permission. +- Non-public direct responses set `content-disposition: attachment`. +- FS responses use `x-accel-redirect` for the configured asset path. +- S3 responses use a presigned URL and an HTTP redirect. + +## File Data + +- `file-data-backend` accepts `legacy-db`, `db`, or `storage`. +- `legacy-db` stores main data in `file.data` and snapshots in `file_change.data`. +- `db` stores encoded data in `file_data.data`. +- `storage` stores encoded data in storage subsystem with `file-data` bucket and keeps `data` nil in `file_data` table. +- The `file_data.metadata.storage-ref-id` value points to the storage object. +- `fdata/upsert!` touches a storage object from incoming metadata before it stores the new row. +- File snapshots use `file_data` for snapshot data and `file_change` for snapshot metadata. diff --git a/.serena/memories/prod-infra/core.md b/.serena/memories/prod-infra/core.md index e86eb69a32..1ec5af0308 100644 --- a/.serena/memories/prod-infra/core.md +++ b/.serena/memories/prod-infra/core.md @@ -6,7 +6,7 @@ Backend (`app.config`, `PENPOT_*` env vars) is parameterized; deployments choose - **PostgreSQL**: durable store. Profiles, teams, files, sessions, audit, `storage_object` metadata, the `task` queue, `scheduled_task` cron registry, migrations. File-data also lives here when the file-data backend is `legacy-db`/`db`. One shared DB across all backends. - **Redis (Valkey-compatible)**: per-backend message bus and cache. Concrete uses: msgbus Pub/Sub for collaborative-editing broadcasts and team/profile-org notifications fired by RPC handlers (`app.rpc.notifications`, `files_update`, `teams`, `websocket`); file-summary cache gated by `enable-redis-cache`; rate-limit counters; and the dispatcher→runner work hand-off list `penpot.worker.queue::`. `PENPOT_REDIS_URI`. -- **Object storage**: backends `:s3` and `:fs`. S3 in prod; devenv uses MinIO. Holds uploaded media, file-data when the file-data backend is `storage`, exports. Backend-side details (resolve, dedup, bucket set, file-data backends): `mem:backend/http-storage-filedata-subtleties`. +- **Object storage**: backends `:s3` and `:fs`. S3 in prod; devenv uses MinIO. Holds uploaded media, file-data when the file-data backend is `storage`, exports. Backend-side details (resolve, dedup, bucket set, object lifecycle, and file-data backends): `mem:backend/storage`. - **SMTP mailer**: invitations, password resets, email verification (sent via the `:sendmail` worker task). - **LDAP** (optional auth provider): helpers in `app.auth.*`, gated by `enable-login-with-ldap`. @@ -30,4 +30,4 @@ Penpot in production lives with both: horizontal-scale deployments accept "exact ## See also - Devenv composition and the ws0-only worker placement: `mem:devenv/core`. -- Storage backend resolution, dedup, file-data lifecycle: `mem:backend/http-storage-filedata-subtleties`. +- Storage backend resolution, dedup, bucket behavior, object lifecycle, and file-data lifecycle: `mem:backend/storage`. From d826c7ac137d3d754f844bf676c2d782047529c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Barrag=C3=A1n=20Merino?= Date: Tue, 18 Aug 2026 12:40:40 +0200 Subject: [PATCH 10/19] :books: Remove architectural constraints related to MCP Server HA --- docs/technical-guide/configuration.md | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index 618b945df6..1541f1d3b8 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -438,7 +438,7 @@ with this flag enabled, the Penpot configuration will disable as well the librar The mechanisms for installing Penpot in HA depend largely on how each infrastructure is managed. In this section, we mention the key factors to consider when replicating a Penpot installation: -The components that can be replicated are the `frontend`, the `backend`, and the `exporter`. +The components that can be replicated are the `frontend`, the `backend`, the `exporter` and the `mcp`. Replication management depends on the infrastructure, whether it's a load balancer or a Kubernetes deployment with HPA. In a high-availability (HA) scenario, managing the state outside of replicas is crucial. This affects the following components: @@ -447,12 +447,6 @@ In a high-availability (HA) scenario, managing the state outside of replicas is - Valkey: Penpot only needs one Valkey instance to function correctly. Due to the nature of the data it manages, replication isn't even essential. - User media storage: This should not be configured with local storage but rather with centralized storage, such as Kubernetes PVC or S3. - -__Since version 2.15.0__ - -Starting with version 2.15, we have introduced the MCP server. Due to architectural constraints, using the MCP server requires running only a single instance of Penpot. -If the MCP server is not installed, then Penpot can scale normally and multiple application instances may be deployed without restrictions. - ## Backend This section enumerates the backend only configuration variables. From ddc98bdd47157a9803efead01888f8a845e41544 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marina=20L=C3=B3pez?= Date: Wed, 19 Aug 2026 07:58:44 +0200 Subject: [PATCH 11/19] :sparkles: Add sso events (#11265) --- backend/src/app/auth/oidc.clj | 85 ++++++++++++++++++- backend/src/app/loggers/audit.clj | 18 ++++ backend/src/app/rpc/commands/nitrate.clj | 17 +++- backend/test/backend_tests/auth_oidc_test.clj | 56 ++++++++++++ .../test/backend_tests/rpc_nitrate_test.clj | 44 +++++++++- .../backend_tests/tasks_telemetry_test.clj | 13 +++ 6 files changed, 226 insertions(+), 7 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index c636aeba24..6cb23fb9de 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -771,6 +771,82 @@ ;; ORG SSO HELPERS ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +(defn- organization-sso-oauth-failure-reason + [error] + (case (d/name error) + "access_denied" "access-denied" + ("temporarily_unavailable" "server_error") "provider-unavailable" + ("invalid_request" "unauthorized_client" "invalid_scope") "invalid-configuration" + "provider-error")) + +(defn- organization-sso-exception-failure-reason + [cause] + (let [data (ex-data cause) + status (or (:response-status data) + (:response-status-code data) + (:http-status data)) + network-error? + (loop [current cause] + (cond + (nil? current) + false + + (or (instance? java.net.ConnectException current) + (instance? java.net.UnknownHostException current) + (instance? java.net.http.HttpTimeoutException current) + (instance? javax.net.ssl.SSLException current)) + true + + (identical? current (ex-cause current)) + false + + :else + (recur (ex-cause current))))] + (if (or network-error? + (and (number? status) (<= 500 status 599))) + "provider-unavailable" + (case (:code data) + :unable-to-fetch-access-token "token-exchange-failed" + :unable-to-retrieve-user-info "user-info-failed" + :incomplete-user-info "incomplete-user-info" + :invalid-sso-config "invalid-configuration" + :unable-to-fetch-sso-jwks "provider-unavailable" + :unable-to-auth "access-denied" + "unexpected-error")))) + +(defn- submit-organization-sso-auth-event + [cfg request profile-id organization-id name & {:keys [failure-reason]}] + (audit/submit cfg {:type "action" + :name name + :profile-id profile-id + :ip-addr (inet/parse-request request) + :props (d/without-nils + {:organization-id organization-id + :failure-reason failure-reason}) + :context (audit/prepare-context-from-request request)})) + +(defn submit-organization-sso-auth-started-event + [cfg request profile-id organization-id] + (submit-organization-sso-auth-event + cfg request profile-id organization-id "organization-sso-auth-started")) + +(defn submit-organization-sso-auth-failed-event + [cfg request profile-id organization-id cause] + (submit-organization-sso-auth-event + cfg request profile-id organization-id "organization-sso-auth-failed" + :failure-reason (organization-sso-exception-failure-reason cause))) + +(defn- submit-organization-sso-oauth-failed-event + [cfg request state-token error] + (try + (let [state (tokens/verify cfg {:token state-token :iss "oidc"})] + (when (:dest-url state) + (submit-organization-sso-auth-event + cfg request (some-> (session/get-session request) :profile-id) + (:organization-id state) "organization-sso-auth-failed" + :failure-reason (organization-sso-oauth-failure-reason error)))) + (catch Throwable _ nil))) + (defn- non-blank-uri [value] (when-not (str/blank? value) value)) @@ -910,6 +986,8 @@ (let [props (-> (or (:props session) {}) (update :sso assoc organization-id exp))] (session/update-session (::session/manager cfg) (assoc session :props props)))) + (submit-organization-sso-auth-event + cfg request (:profile-id session) organization-id "organization-sso-auth-succeeded") (redirect-response dest-url)) (catch Throwable cause (let [{:keys [code]} (ex-data cause)] @@ -922,6 +1000,9 @@ (l/err :hint "unexpected error on organization sso callback" :organization-id (:organization-id state) :cause cause)))) + (submit-organization-sso-auth-failed-event + cfg request (some-> (session/get-session request) :profile-id) + (:organization-id state) cause) (let [organization-id (:organization-id state) organization-name (:name (nitrate/call cfg :get-organization-summary {:organization-id organization-id}))] (redirect-with-organization-sso-error @@ -932,7 +1013,9 @@ (defn- callback-handler [cfg {:keys [params] :as request}] (if-let [error (get params :error)] - (redirect-with-error "unable-to-auth" error) + (do + (submit-organization-sso-oauth-failed-event cfg request (:state params) error) + (redirect-with-error "unable-to-auth" error)) (try (let [code (get params :code) state (get params :state) diff --git a/backend/src/app/loggers/audit.clj b/backend/src/app/loggers/audit.clj index f68209255b..6ded7befaf 100644 --- a/backend/src/app/loggers/audit.clj +++ b/backend/src/app/loggers/audit.clj @@ -36,6 +36,16 @@ (def ^:private filter-auth-events #{"login-with-oidc" "login-with-password" "register-profile" "update-profile"}) +(def ^:private organization-sso-failure-reasons + #{"access-denied" + "provider-unavailable" + "invalid-configuration" + "provider-error" + "token-exchange-failed" + "user-info-failed" + "incomplete-user-info" + "unexpected-error"}) + (def ^:private safe-backend-context-keys #{:version :initiator @@ -297,6 +307,14 @@ (defn filter-telemetry-props [{:keys [source name props type] :as params}] (cond + (and (= source "backend") + (= name "organization-sso-auth-failed")) + (let [props' (into {} xf:filter-telemetry-props props) + props' (cond-> props' + (contains? organization-sso-failure-reasons (:failure-reason props)) + (assoc :failure-reason (:failure-reason props)))] + (assoc params :props props')) + (or (and (= source "frontend") (= type "identify")) (and (= source "backend") diff --git a/backend/src/app/rpc/commands/nitrate.clj b/backend/src/app/rpc/commands/nitrate.clj index c48834662e..a476ce0dbf 100644 --- a/backend/src/app/rpc/commands/nitrate.clj +++ b/backend/src/app/rpc/commands/nitrate.clj @@ -698,10 +698,19 @@ (if authorized {:authorized true :reason :sso-satisfied} (if (oidc/organization-sso-discovery-uri sso) - {:authorized false - :redirect-uri (oidc/build-organization-sso-auth-redirect-uri cfg sso - :dest-url url - :organization-id organization-id)} + (try + (let [redirect-uri (oidc/build-organization-sso-auth-redirect-uri + cfg sso + :dest-url url + :organization-id organization-id) + organization-id (or organization-id (:organization-id sso))] + (oidc/submit-organization-sso-auth-started-event + cfg request profile-id organization-id) + {:authorized false :redirect-uri redirect-uri}) + (catch Throwable cause + (oidc/submit-organization-sso-auth-failed-event + cfg request profile-id (or organization-id (:organization-id sso)) cause) + (throw cause))) {:authorized false :redirect-uri nil})))) {:authorized true :reason :sso-satisfied})) diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index 62f04fd546..b99de502c4 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -385,6 +385,9 @@ (def ^:private test-profile-id #uuid "11111111-1111-1111-1111-111111111111") +(def ^:private test-organization-id + #uuid "22222222-2222-2222-2222-222222222222") + (def ^:private test-profile {:id test-profile-id :is-active true @@ -519,6 +522,59 @@ (t/is (= 302 (::yres/status result))) (t/is (.contains loc "error=unable-to-auth"))))))) +(t/deftest organization-sso-callback-success-emits-succeeded + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + state (make-state-token cfg {:dest-url "https://penpot.example.com/#/workspace" + :organization-id test-organization-id}) + request (default-request cfg :state state) + events (atom [])] + (with-redefs [app.nitrate/call (constantly {:active true}) + app.auth.oidc/prepare-organization-sso-provider (constantly {:type "oidc"}) + app.auth.oidc/get-info (constantly {}) + app.loggers.audit/submit (fn [_cfg event] (swap! events conj event))] + (let [result (#'oidc/callback-handler cfg request)] + (t/is (= "https://penpot.example.com/#/workspace" (redirect-location result))) + (t/is (= ["organization-sso-auth-succeeded"] (mapv :name @events))) + (t/is (= test-organization-id (get-in (first @events) [:props :organization-id]))))))) + +(t/deftest organization-sso-callback-error-emits-failed + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + state (make-state-token cfg {:dest-url "https://penpot.example.com/#/workspace" + :organization-id test-organization-id}) + request (default-request cfg :state state) + events (atom [])] + (with-redefs [app.nitrate/call (fn [_cfg method _params] + (case method + :get-organization-sso {:active true} + :get-organization-summary {:name "Organization"})) + app.auth.oidc/prepare-organization-sso-provider (constantly {:type "oidc"}) + app.auth.oidc/get-info (fn [& _] + (ex/raise :type :internal + :code :unable-to-retrieve-user-info)) + app.loggers.audit/submit (fn [_cfg event] (swap! events conj event))] + (#'oidc/callback-handler cfg request) + (t/is (= ["organization-sso-auth-failed"] (mapv :name @events))) + (t/is (= {:organization-id test-organization-id + :failure-reason "user-info-failed"} + (:props (first @events))))))) + +(t/deftest organization-sso-oauth-error-emits-failed-without-changing-redirect + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + state (make-state-token cfg {:dest-url "https://penpot.example.com/#/workspace" + :organization-id test-organization-id}) + request (assoc-in (default-request cfg :state state) [:params :error] "access_denied") + events (atom [])] + (binding [cf/config {:public-uri "http://localhost:3449"}] + (with-redefs [app.loggers.audit/submit (fn [_cfg event] (swap! events conj event))] + (let [result (#'oidc/callback-handler cfg request) + loc (redirect-location result)] + (t/is (.contains loc "error=unable-to-auth")) + (t/is (.contains loc "hint=access_denied")) + (t/is (= ["organization-sso-auth-failed"] (mapv :name @events))) + (t/is (= {:organization-id test-organization-id + :failure-reason "access-denied"} + (:props (first @events))))))))) + (t/deftest prepare-organization-sso-provider-does-not-skip-ssrf-check (t/testing "organization SSO provider must use SSRF protection" (let [captured-params (atom nil)] diff --git a/backend/test/backend_tests/rpc_nitrate_test.clj b/backend/test/backend_tests/rpc_nitrate_test.clj index d2ce0043bf..90b746e2a1 100644 --- a/backend/test/backend_tests/rpc_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_nitrate_test.clj @@ -148,6 +148,8 @@ team (th/create-team* 1 {:profile-id (:id team-owner)}) organization-id (uuid/random) redirect-uri "https://idp.example.com/authorize" + redirect-options (atom nil) + started-event (atom nil) params (with-meta {::th/type :check-nitrate-sso ::rpc/profile-id (:id team-owner) @@ -161,12 +163,50 @@ organization-id (:id team-owner)) oidc/build-organization-sso-auth-redirect-uri - (constantly redirect-uri)] + (fn [_cfg _sso & options] + (reset! redirect-options (apply hash-map options)) + redirect-uri) + oidc/submit-organization-sso-auth-started-event + (fn [_cfg _request profile-id received-organization-id] + (reset! started-event {:profile-id profile-id + :organization-id received-organization-id}))] (let [out (th/command! params)] (t/is (th/success? out)) (t/is (= {:authorized false :redirect-uri redirect-uri} - (:result out)))))))) + (:result out))) + (t/is (= #{:dest-url :organization-id} (set (keys @redirect-options)))) + (t/is (= "https://penpot.example.com/#/workspace" (str (:dest-url @redirect-options)))) + (t/is (nil? (:organization-id @redirect-options))) + (t/is (= {:profile-id (:id team-owner) + :organization-id organization-id} + @started-event))))))) + +(t/deftest check-nitrate-sso-reports-redirect-failure + (let [profile (th/create-profile* 1 {:is-active true}) + organization-id (uuid/random) + cause (ex-info "provider unavailable" {:response-status-code 503}) + reported (atom nil) + params (with-meta + {::th/type :check-nitrate-sso + ::rpc/profile-id (:id profile) + :organization-id organization-id + :url "https://penpot.example.com/#/workspace"} + {::http/request {}})] + (binding [cf/flags (conj cf/flags :admin-console)] + (with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock organization-id) + oidc/build-organization-sso-auth-redirect-uri (fn [& _] (throw cause)) + oidc/submit-organization-sso-auth-failed-event + (fn [_cfg _request profile-id received-organization-id received-cause] + (reset! reported {:profile-id profile-id + :organization-id received-organization-id + :cause received-cause}))] + (let [out (th/command! params)] + (t/is (not (th/success? out))) + (t/is (= {:profile-id (:id profile) + :organization-id organization-id + :cause cause} + @reported))))))) (t/deftest check-nitrate-sso-keeps-gate-for-non-member-organization-owner (let [team-owner (th/create-profile* 1 {:is-active true}) diff --git a/backend/test/backend_tests/tasks_telemetry_test.clj b/backend/test/backend_tests/tasks_telemetry_test.clj index 07b8f7c7f6..e3f57647df 100644 --- a/backend/test/backend_tests/tasks_telemetry_test.clj +++ b/backend/test/backend_tests/tasks_telemetry_test.clj @@ -710,6 +710,19 @@ (t/is (not (contains? (:props result) :route))) (t/is (not (contains? (:props result) :label))))) +(t/deftest test-filter-telemetry-props-organization-sso-failure-keeps-reason + (let [ftp (ns-resolve 'app.loggers.audit 'filter-telemetry-props) + organization-id (uuid/next) + result (ftp {:source "backend" + :name "organization-sso-auth-failed" + :type "action" + :props {:organization-id organization-id + :failure-reason "access-denied" + :unsafe-label "should-be-stripped"}})] + (t/is (= {:organization-id organization-id + :failure-reason "access-denied"} + (:props result))))) + (t/deftest test-filter-telemetry-props-navigate-keeps-route-and-ids ;; Frontend navigate events keep specific routing keys: :route, ;; :file-id, :team-id, :page-id. These ids are strings because From 4d90fe9126b1ee4ed6676ff9e81c114dcf6192c3 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 12:20:37 +0200 Subject: [PATCH 12/19] :sparkles: Add advisories access helper to gh tool --- .serena/memories/scripts/gh.md | 25 +++++++ scripts/gh.py | 121 ++++++++++++++++++++++++++++++++- 2 files changed, 144 insertions(+), 2 deletions(-) diff --git a/.serena/memories/scripts/gh.md b/.serena/memories/scripts/gh.md index ed6adfa8f1..58f73e997d 100644 --- a/.serena/memories/scripts/gh.md +++ b/.serena/memories/scripts/gh.md @@ -9,6 +9,7 @@ repository via GraphQL and REST APIs through the authenticated `gh` CLI. - Finding issues with no milestone. - Fetching PR details by number or by milestone. - Comparing milestone issues against CHANGES.md to find missing entries. +- Listing or inspecting GitHub Security Advisories (GHSA). ## Prerequisites @@ -72,6 +73,30 @@ python3 scripts/gh.py prs --milestone "2.16.0" --state all **Output**: JSON array to stdout; progress to stderr. +### `advisories` + +List or inspect GitHub Security Advisories for the repository. + +```bash +# List all advisories (summary view) +python3 scripts/gh.py advisories + +# Filter by severity +python3 scripts/gh.py advisories --severity critical + +# Filter by state +python3 scripts/gh.py advisories --state triage + +# Get full detail for a single advisory +python3 scripts/gh.py advisories GHSA-xvj6-fh9w-gjw7 +``` + +**Summary output fields**: ghsa_id, cve_id, severity, cvss_score, state, summary, cwes, published_at, closed_at, url. + +**Detail output** (single advisory) adds: description, vulnerabilities (package, version ranges), credits, timestamps. + +**Output**: JSON to stdout; progress to stderr. + ## Key principles - All output is JSON — pipe into `jq` or other tools for further processing. diff --git a/scripts/gh.py b/scripts/gh.py index 7017389d52..c7e1f87aca 100755 --- a/scripts/gh.py +++ b/scripts/gh.py @@ -5,8 +5,9 @@ gh.py — Multi-purpose CLI helper for penpot/penpot GitHub operations. Uses GitHub GraphQL and REST APIs via the authenticated ``gh`` CLI. Subcommands: - issues List issues in a milestone (or unassigned with milestone=none) - prs Fetch details for one or more PRs (by number or milestone) + issues List issues in a milestone (or unassigned with milestone=none) + prs Fetch details for one or more PRs (by number or milestone) + advisories List or inspect GitHub security advisories Usage: python3 scripts/gh.py issues (default: state=closed) @@ -23,6 +24,9 @@ Usage: cat prs.txt | python3 scripts/gh.py prs --stdin python3 scripts/gh.py prs --milestone "2.16.0" (default: state=merged) python3 scripts/gh.py prs --milestone "2.16.0" --state all + python3 scripts/gh.py advisories (list all advisories) + python3 scripts/gh.py advisories --severity critical (filter by severity) + python3 scripts/gh.py advisories GHSA-xvj6-fh9w-gjw7 (single advisory detail) Prerequisites: - gh CLI authenticated (gh auth status) @@ -63,6 +67,16 @@ def run_gh_graphql(query: str, variables: dict) -> Any: return body["data"] +def run_gh_rest(path: str) -> Any: + """Run a REST API call via ``gh api``.""" + cmd = ["gh", "api", path] + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + print(f"gh error: {result.stderr}", file=sys.stderr) + sys.exit(1) + return json.loads(result.stdout) + + # ───────────────────────────────────────────── # Shared: milestone lookup # ───────────────────────────────────────────── @@ -581,6 +595,93 @@ def cmd_prs(args: argparse.Namespace) -> None: print(json.dumps(all_results, indent=2)) +# ───────────────────────────────────────────── +# Subcommand: advisories +# ───────────────────────────────────────────── + + +def fetch_advisories() -> list[dict]: + """Fetch all security advisories for the repository via REST API.""" + return run_gh_rest(f"repos/{REPO}/security-advisories") + + +def fetch_advisory(ghsa_id: str) -> dict: + """Fetch a single security advisory by GHSA ID.""" + return run_gh_rest(f"repos/{REPO}/security-advisories/{ghsa_id}") + + +def format_advisory_summary(adv: dict) -> dict: + """Extract a summary view of an advisory.""" + return { + "ghsa_id": adv["ghsa_id"], + "cve_id": adv.get("cve_id"), + "severity": adv.get("severity"), + "cvss_score": (adv.get("cvss") or {}).get("score"), + "state": adv.get("state"), + "summary": adv.get("summary"), + "cwes": [c["cwe_id"] for c in adv.get("cwes", [])], + "published_at": adv.get("published_at"), + "closed_at": adv.get("closed_at"), + "url": adv.get("html_url"), + } + + +def format_advisory_detail(adv: dict) -> dict: + """Extract full detail view of an advisory.""" + summary = format_advisory_summary(adv) + summary["description"] = adv.get("description") + summary["vulnerabilities"] = [ + { + "package": v.get("package", {}).get("name"), + "vulnerable_version_range": v.get("vulnerable_version_range"), + "patched_versions": v.get("patched_versions"), + } + for v in adv.get("vulnerabilities", []) + ] + summary["credits"] = [ + {"login": c.get("user", {}).get("login"), "type": c.get("type")} + for c in adv.get("credits_detailed", []) + ] + summary["created_at"] = adv.get("created_at") + summary["updated_at"] = adv.get("updated_at") + summary["withdrawn_at"] = adv.get("withdrawn_at") + return summary + + +def cmd_advisories(args: argparse.Namespace) -> None: + """Handle the ``advisories`` subcommand.""" + + # ── Single advisory detail ────────────────────────────── + if args.ghsa_id: + ghsa_id = args.ghsa_id.upper() + if not ghsa_id.startswith("GHSA-"): + ghsa_id = f"GHSA-{ghsa_id}" + print(f"Fetching advisory {ghsa_id}...", file=sys.stderr) + adv = fetch_advisory(ghsa_id) + print(json.dumps(format_advisory_detail(adv), indent=2)) + return + + # ── List all advisories ───────────────────────────────── + print("Fetching security advisories...", file=sys.stderr) + advisories = fetch_advisories() + print(f"Fetched {len(advisories)} advisories", file=sys.stderr) + + results = [format_advisory_summary(adv) for adv in advisories] + + # Apply filters + if args.severity: + sev = args.severity.lower() + results = [r for r in results if (r.get("severity") or "").lower() == sev] + print(f"After severity filter ({sev}): {len(results)} advisories", file=sys.stderr) + + if args.state: + st = args.state.lower() + results = [r for r in results if (r.get("state") or "").lower() == st] + print(f"After state filter ({st}): {len(results)} advisories", file=sys.stderr) + + print(json.dumps(results, indent=2)) + + # ───────────────────────────────────────────── # CLI entrypoint # ───────────────────────────────────────────── @@ -645,6 +746,22 @@ def main() -> None: ) p_prs.set_defaults(func=cmd_prs) + # --- advisories --- + p_adv = sub.add_parser("advisories", help="List or inspect GitHub security advisories") + p_adv.add_argument( + "ghsa_id", nargs="?", + help="GHSA ID to fetch (e.g. 'GHSA-xvj6-fh9w-gjw7'); omit to list all" + ) + p_adv.add_argument( + "--severity", choices=["critical", "high", "medium", "low"], + help="Filter by severity level" + ) + p_adv.add_argument( + "--state", choices=["triage", "draft", "published", "closed", "withdrawn"], + help="Filter by advisory state" + ) + p_adv.set_defaults(func=cmd_advisories) + args = parser.parse_args() args.func(args) From 5080a90f760df217d79dea7180f621d2b52b8ade Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Wed, 19 Aug 2026 12:33:33 +0200 Subject: [PATCH 13/19] :lipstick: Change nitrate activation code texts (#11237) --- .../nitrate_code_activation_modal.cljs | 21 ++++++++++++------- .../src/app/main/ui/nitrate/nitrate_form.cljs | 6 +++--- frontend/translations/en.po | 13 +++++++----- frontend/translations/es.po | 13 +++++++----- 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/frontend/src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs b/frontend/src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs index 0fc1b86b03..876852198e 100644 --- a/frontend/src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs +++ b/frontend/src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs @@ -111,11 +111,16 @@ :value (tr "nitrate.code-activation.submit") :on-click on-accept}]] [:div {:class (stl/css :footer-text)} - (tr "nitrate.code-activation.footer-before") - [:a {:class (stl/css :link) - :on-click on-download-request-click} - (tr "nitrate.code-activation.footer-link")] - (tr "nitrate.code-activation.footer-after") " " - [:a {:class (stl/css :link) - :href "mailto:sales@nitrate.com"} - "sales@nitrate.com"]]]]])) + [:div {:class (stl/css :code-label)} (tr "nitrate.code-activation.footer-title")] + [:div + + [:a {:class (stl/css :link) + :on-click on-download-request-click} + (tr "nitrate.code-activation.footer-download")]] + [:div + (tr "nitrate.code-activation.footer-after") " " + [:a {:class (stl/css :link) + :href "mailto:sales@nitrate.com"} + "sales@nitrate.com"] + " " + (tr "nitrate.code-activation.footer-before")]]]]])) diff --git a/frontend/src/app/main/ui/nitrate/nitrate_form.cljs b/frontend/src/app/main/ui/nitrate/nitrate_form.cljs index 3ad31a1977..5b983ef888 100644 --- a/frontend/src/app/main/ui/nitrate/nitrate_form.cljs +++ b/frontend/src/app/main/ui/nitrate/nitrate_form.cljs @@ -102,9 +102,9 @@ (tr "nitrate.form.cancel-anytime")]]] [:p {:class (stl/css :modal-text-medium)} - (tr "nitrate.form.subscribe-with-code") " " [:a {:class (stl/css :link) - :on-click on-activate-click} - (tr "nitrate.form.enter-code")]] + [:a {:class (stl/css :link) + :on-click on-activate-click} + (tr "nitrate.form.subscribe-with-code")]] [:p {:class (stl/css :modal-text-medium)} [:a {:class (stl/css :link) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index f2b57e315f..3175d5a1c3 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -4407,14 +4407,17 @@ msgid "nitrate.modal-success.title" msgstr "Welcome to Enterprise!" #: src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs:104 -msgid "nitrate.code-activation.footer-before" -msgstr "Need a code? Download your " +msgid "nitrate.code-activation.footer-title" +msgstr "Need a code?" -msgid "nitrate.code-activation.footer-link" -msgstr "activation code request" +msgid "nitrate.code-activation.footer-download" +msgstr "Download request" msgid "nitrate.code-activation.footer-after" -msgstr " and contact us:" +msgstr "Send the file to" + +msgid "nitrate.code-activation.footer-before" +msgstr "and we will send you your code." #: src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs:86 msgid "nitrate.code-activation.input-label" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index eacd139708..4044119275 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -4278,14 +4278,17 @@ msgid "nitrate.modal-success.title" msgstr "¡Bienvenido a Enterprise!" #: src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs:104 -msgid "nitrate.code-activation.footer-before" -msgstr "¿Necesitas un código? Descarga tu " +msgid "nitrate.code-activation.footer-title" +msgstr "¿Necesitas un código?" -msgid "nitrate.code-activation.footer-link" -msgstr "solicitud de código de activación" +msgid "nitrate.code-activation.footer-download" +msgstr " Descargar solicitud" msgid "nitrate.code-activation.footer-after" -msgstr " y contáctanos:" +msgstr "Mánda el fichero a" + +msgid "nitrate.code-activation.footer-before" +msgstr "y te enviaremos tu código." #: src/app/main/ui/nitrate/nitrate_code_activation_modal.cljs:86 msgid "nitrate.code-activation.input-label" From fda6d56139353be2e2290c96983de509b6f5c038 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 13:00:01 +0200 Subject: [PATCH 14/19] :books: Update AGENTS.md file --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index e542f93eb2..d4d1e238e7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,6 +34,13 @@ Skipping this step is the #1 cause of incorrect or incomplete work. --- +## Auto-triggers + +- **Security advisory URL pasted** — When the user pastes a URL matching + `github.com/penpot/penpot/security/advisories/GHSA-*`, extract the GHSA ID + from the URL and run `python3 scripts/gh.py advisories ` to fetch + full advisory details before proceeding. + ## Writing Rules Use the `ste` skill when the user explicitly requests STE, `/ste`, or ASD-STE100. @@ -119,4 +126,5 @@ precision while maintaining a strong focus on maintainability and performance. - `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`. +- `scripts/gh.py` — Multi-purpose GitHub CLI helper. Subcommands: `issues` (list issues in a milestone), `prs` (fetch PR details), `advisories` (list/inspect security advisories). See `python3 scripts/gh.py --help`. From aa3bc1ae984577f0354d4270d2546e15985f4bcf Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 13:29:04 +0200 Subject: [PATCH 15/19] :bug: Fix linear gradients in SVG text exports (#11272) * :bug: Use gradient type instead of export type in SVG renderer data->gradient-def was comparing the render `type` parameter (:svg, :png, :pdf) against "linear" to decide between linearGradient and radialGradient elements. Since the export type is never "linear", the comparison always fell through to radialGradient, causing all linear gradients to be exported as radial in SVG output. Read the gradient type from the data map instead: (get-in data ["gradient" "type"]) Closes #5972 * :bug: Add SVG gradient export regression test Extract SVG gradient definition generation from the renderer so it can be tested directly. Add exporter test build wiring and cover both linear and radial gradient output. AI-assisted-by: gpt-5.6-luna * :sparkles: Standardize exporter testing workflow Align exporter scripts with the frontend testing pattern. Add a dedicated GitHub Actions workflow and document the canonical exporter commands in Serena memories. AI-assisted-by: gpt-5.6-luna * :sparkles: Add focused exporter test execution Mirror frontend test-runner behavior for focused namespaces and test vars. Support --focus, --log-level, and --help, and document the commands. AI-assisted-by: gpt-5.6-luna * :bug: Replace shell exec with execFile in exporter Replace child_process.exec with execFile to eliminate shell interpretation. Add hex color validation in exporter and frontend to reject malformed input before command construction. This fixes GHSA-4f36-m4hj-cv86 (CVSS 9.9 Critical), an authenticated OS command injection vulnerability where malicious fill-color values could execute arbitrary commands in the exporter container. Defense in depth: - Layer 1: execFile passes arguments directly without shell parsing - Layer 2: Exporter validates colors with strict hex regex - Layer 3: Frontend filters invalid colors before DOM emission All three independent reporters' attack vectors are addressed: - Quote breakout (lyhtheori) - Command substitution (B1gN0Se) - Path traversal (KimiSecurityTeam) AI-assisted-by: qwen3.7-plus * :bug: Use existing hex-color-string? and fix test path mismatch Address code review feedback: - Replace duplicated hex-color-rx and valid-hex-color? with existing hex-color-string? from app.common.types.color - Fix RCE test to use marker path in payload instead of hardcoded /tmp/pwned AI-assisted-by: qwen3.7-plus --------- Co-authored-by: Sumit Ridhal --- .github/workflows/tests-exporter.yml | 58 ++++++ .serena/memories/exporter/core.md | 5 +- .serena/memories/exporter/testing.md | 16 ++ exporter/package.json | 9 +- exporter/scripts/test | 7 + exporter/scripts/test-quiet.js | 29 +++ exporter/shadow-cljs.edn | 10 +- exporter/src/app/handlers/export_frames.cljs | 2 +- exporter/src/app/renderer/bitmap.cljs | 2 +- exporter/src/app/renderer/svg.cljs | 38 ++-- exporter/src/app/renderer/svg_gradient.cljs | 32 ++++ exporter/src/app/util/shell.cljs | 16 +- .../exporter_tests/renderer_svg_test.cljs | 25 +++ exporter/test/exporter_tests/runner.cljs | 172 ++++++++++++++++++ exporter/test/exporter_tests/shell_test.cljs | 70 +++++++ .../src/app/main/ui/shapes/text/fo_text.cljs | 12 +- scripts/ci | 10 +- 17 files changed, 462 insertions(+), 51 deletions(-) create mode 100644 .github/workflows/tests-exporter.yml create mode 100644 .serena/memories/exporter/testing.md create mode 100755 exporter/scripts/test create mode 100644 exporter/scripts/test-quiet.js create mode 100644 exporter/src/app/renderer/svg_gradient.cljs create mode 100644 exporter/test/exporter_tests/renderer_svg_test.cljs create mode 100644 exporter/test/exporter_tests/runner.cljs create mode 100644 exporter/test/exporter_tests/shell_test.cljs diff --git a/.github/workflows/tests-exporter.yml b/.github/workflows/tests-exporter.yml new file mode 100644 index 0000000000..1ed37d95c8 --- /dev/null +++ b/.github/workflows/tests-exporter.yml @@ -0,0 +1,58 @@ +name: "CI: Exporter" + +defaults: + run: + shell: bash + +on: + pull_request: + paths: + - 'exporter/**' + - 'common/**' + + types: + - opened + - synchronize + - ready_for_review + + push: + branches: + - develop + - staging + + paths: + - 'exporter/**' + - 'common/**' + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + test-exporter: + if: ${{ !github.event.pull_request.draft }} + name: "Exporter Tests" + runs-on: penpot-runner-02 + container: + image: penpotapp/devenv:latest + volumes: + - /var/cache/github-runner/m2:/root/.m2 + - /var/cache/github-runner/gitlib:/root/.gitlibs + + steps: + - name: Checkout repository + uses: actions/checkout@v6 + + - name: Lint + working-directory: ./exporter + run: | + corepack enable; + corepack install; + pnpm install; + pnpm run check-fmt:clj + pnpm run lint:clj + + - name: Tests + working-directory: ./exporter + run: | + ./scripts/test diff --git a/.serena/memories/exporter/core.md b/.serena/memories/exporter/core.md index 3bcf784f49..9b7078045b 100644 --- a/.serena/memories/exporter/core.md +++ b/.serena/memories/exporter/core.md @@ -5,9 +5,10 @@ ## Layout and commands - Source: `exporter/src/`; config: `deps.edn`, `shadow-cljs.edn`, `package.json`; runtime helpers/assets: `vendor/`, `scripts/`. -- From `exporter/`: setup `./scripts/setup`; watch `pnpm run watch` or `pnpm run watch:app`; production build `pnpm run build`; lint `pnpm run lint`; format check/fix `pnpm run check-fmt` / `pnpm run fmt`. +- From `exporter/`: setup `./scripts/setup`; watch `pnpm run watch` or `pnpm run watch:app`; production build `pnpm run build`; test bundle `pnpm run build:test`; tests `pnpm run test` or `pnpm run test:quiet`; lint `pnpm run lint:clj`; format check/fix `pnpm run check-fmt:clj` / `pnpm run fmt:clj`. - Because exporter consumes `common/`, shared file/shape/model changes may need exporter verification even when the immediate change is not under `exporter/`. - Cross-cutting testing principles and anti-patterns: `mem:testing`. +- Exporter test conventions and CI: `mem:exporter/testing`. ## HTTP and browser pool @@ -31,4 +32,4 @@ - WebP is produced by taking a PNG screenshot and converting it with ImageMagick. - SVG export rasterizes text foreignObjects to PNG, converts through PPM/color masks/potrace, and reassembles SVG paths. It also replaces non-breaking spaces for SVG compatibility and drops empty defs/paths. - PDF export injects `@page` sizing through raw browser `evaluate` JavaScript; that code cannot rely on CLJS runtime helpers. -- Temporary resources schedule local deletion, then uploads POST to `/api/management/methods/upload-tempfile` with `X-Shared-Key: exporter ` and Bearer auth. \ No newline at end of file +- Temporary resources schedule local deletion, then uploads POST to `/api/management/methods/upload-tempfile` with `X-Shared-Key: exporter ` and Bearer auth. diff --git a/.serena/memories/exporter/testing.md b/.serena/memories/exporter/testing.md new file mode 100644 index 0000000000..189c1e852c --- /dev/null +++ b/.serena/memories/exporter/testing.md @@ -0,0 +1,16 @@ +# Exporter Testing + +- READ `mem:testing` first. +- Tests use `cljs.test` and live under `exporter/test/exporter_tests/`. +- Register every test namespace in `exporter-tests.runner`. +- From `exporter/`: `pnpm run build:test` builds the Node test bundle without running tests. +- From `exporter/`: `pnpm run test` builds and runs tests with full output. +- From `exporter/`: `pnpm run test:quiet` builds and runs tests with reduced build output. +- After `build:test`, reuse the compiled bundle with `node target/tests/test.js`. +- For iterative focused runs, build once and reuse the compiled bundle. +- Focus a test namespace with `node target/tests/test.js --focus exporter-tests.renderer-svg-test`. +- Focus a test var with `node target/tests/test.js --focus exporter-tests.renderer-svg-test/creates-the-correct-gradient-element`. +- Set app log level by appending `--log-level warn` (or `trace|debug|info|warn|error`). +- `test:quiet` accepts forwarded options but rebuilds the bundle; prefer the direct runner after `build:test` for focused runs. +- From `exporter/`: `pnpm run check-fmt:clj` checks ClojureScript formatting. +- From `exporter/`: `pnpm run lint:clj` runs ClojureScript linting. diff --git a/exporter/package.json b/exporter/package.json index 83518eabee..e52fb9fd12 100644 --- a/exporter/package.json +++ b/exporter/package.json @@ -34,8 +34,11 @@ "watch": "pnpm run watch:app", "build:app": "clojure -M:dev:shadow-cljs release main", "build": "pnpm run clear:shadow-cache && pnpm run build:app", - "fmt": "cljfmt fix --parallel=true src/", - "check-fmt": "cljfmt check --parallel=true src/", - "lint": "clj-kondo --parallel --lint src/" + "fmt:clj": "cljfmt fix --parallel=true src/ test/", + "check-fmt:clj": "cljfmt check --parallel=true src/ test/", + "lint:clj": "clj-kondo --parallel --lint src/ test/", + "build:test": "clojure -M:dev:shadow-cljs compile test", + "test": "pnpm run build:test && node target/tests/test.js", + "test:quiet": "node ./scripts/test-quiet.js" } } diff --git a/exporter/scripts/test b/exporter/scripts/test new file mode 100755 index 0000000000..6402c5afd1 --- /dev/null +++ b/exporter/scripts/test @@ -0,0 +1,7 @@ +#!/usr/bin/env bash + +set -ex +corepack enable; +corepack install; +pnpm install; +pnpm run test; diff --git a/exporter/scripts/test-quiet.js b/exporter/scripts/test-quiet.js new file mode 100644 index 0000000000..b1be0dd682 --- /dev/null +++ b/exporter/scripts/test-quiet.js @@ -0,0 +1,29 @@ +import { spawnSync } from "node:child_process"; + +const BUILD_STEPS = [ + { label: "Building test bundle", cmd: "pnpm", args: ["run", "build:test"] }, +]; + +const progress = (msg) => process.stderr.write(`${msg}\n`); + +for (const step of BUILD_STEPS) { + progress(`${step.label}...`); + const result = spawnSync(step.cmd, step.args, { + stdio: ["ignore", "pipe", "pipe"], + maxBuffer: 64 * 1024 * 1024, + }); + if (result.status !== 0) { + progress(`${step.label} failed`); + if (result.stdout?.length) process.stdout.write(result.stdout); + if (result.stderr?.length) process.stderr.write(result.stderr); + process.exit(result.status ?? 1); + } +} + +progress("Running tests..."); +const result = spawnSync( + "node", + ["target/tests/test.js", ...process.argv.slice(2)], + { stdio: "inherit" }, +); +process.exit(result.status ?? 1); diff --git a/exporter/shadow-cljs.edn b/exporter/shadow-cljs.edn index ae963cf311..076e6a10f6 100644 --- a/exporter/shadow-cljs.edn +++ b/exporter/shadow-cljs.edn @@ -31,4 +31,12 @@ :pseudo-names true :pretty-print true :anon-fn-naming-policy :off - :source-map-detail-level :all}}}}} + :source-map-detail-level :all}}} + + :test + {:target :esm + :output-dir "target/tests" + :runtime :node + :js-options {:js-provider :import} + :modules + {:test {:init-fn exporter-tests.runner/-main}}}}} diff --git a/exporter/src/app/handlers/export_frames.cljs b/exporter/src/app/handlers/export_frames.cljs index cf263d8e00..658456059d 100644 --- a/exporter/src/app/handlers/export_frames.cljs +++ b/exporter/src/app/handlers/export_frames.cljs @@ -117,7 +117,7 @@ [file-id paths] (p/let [prefix (str/concat "penpot.pdfunite." file-id ".") path (sh/tempfile :prefix prefix :suffix ".pdf")] - (sh/run-cmd! (str "pdfunite " (str/join " " paths) " " path)) + (apply sh/run-cmd! "pdfunite" (conj (vec paths) path)) path)) (defn- move-file diff --git a/exporter/src/app/renderer/bitmap.cljs b/exporter/src/app/renderer/bitmap.cljs index c2720eb025..e04c60076b 100644 --- a/exporter/src/app/renderer/bitmap.cljs +++ b/exporter/src/app/renderer/bitmap.cljs @@ -38,7 +38,7 @@ :webp (p/let [png-path (sh/tempfile :prefix "penpot.tmp.bitmap." :suffix ".png")] ;; playwright only supports jpg and png, we need to convert it afterwards (bw/screenshot node {:omit-background? true :type :png :path png-path}) - (sh/run-cmd! (str "convert " png-path " -quality 100 WEBP:" path)))) + (sh/run-cmd! "convert" png-path "-quality" "100" (str "WEBP:" path)))) (on-object (assoc object :path path)))) (render [uri page] diff --git a/exporter/src/app/renderer/svg.cljs b/exporter/src/app/renderer/svg.cljs index c9fee2f764..0db4bc0cf8 100644 --- a/exporter/src/app/renderer/svg.cljs +++ b/exporter/src/app/renderer/svg.cljs @@ -10,9 +10,12 @@ ["xml-js" :as xml] [app.browser :as bw] [app.common.data :as d] + [app.common.exceptions :as ex] [app.common.logging :as l] + [app.common.types.color :as ctc] [app.common.uri :as u] [app.config :as cf] + [app.renderer.svg-gradient :as svg-gradient] [app.util.mime :as mime] [app.util.shell :as sh] [clojure.walk :as walk] @@ -125,19 +128,23 @@ (letfn [(convert-to-ppm [pngpath] (let [ppmpath (str/concat pngpath "origin.ppm")] (l/trace :fn :convert-to-ppm :path ppmpath) - (-> (sh/run-cmd! (str "convert " pngpath " " ppmpath)) + (-> (sh/run-cmd! "convert" pngpath ppmpath) (p/then (constantly ppmpath))))) (trace-color-mask [pbmpath] (l/trace :fn :trace-color-mask :pbmpath pbmpath) (let [svgpath (str/concat pbmpath ".svg")] - (-> (sh/run-cmd! (str "potrace --flat -b svg " pbmpath " -o " svgpath)) + (-> (sh/run-cmd! "potrace" "--flat" "-b" "svg" pbmpath "-o" svgpath) (p/then (constantly svgpath))))) (generate-color-layer [ppmpath color] + (when-not (ctc/hex-color-string? color) + (ex/raise :type :validation + :code :invalid-color + :hint (str "invalid hex color: " color))) (l/trace :fn :generate-color-layer :ppmpath ppmpath :color color) (let [pbmpath (str/concat ppmpath ".mask-" (subs color 1) ".pbm")] - (-> (sh/run-cmd! (str/format "ppmcolormask \"%s\" %s" color ppmpath)) + (-> (sh/run-cmd! "ppmcolormask" color ppmpath) (p/then (fn [stdout] (-> (sh/write-file! pbmpath stdout) (p/then (constantly pbmpath))))) @@ -166,33 +173,11 @@ :else (update node "attributes" assoc "fill" color)))) - (get-stops [data] - (->> (get-in data ["gradient" "stops"]) - (mapv (fn [stop-data] - {"type" "element" - "name" "stop" - "attributes" {"offset" (get stop-data "offset") - "stop-color" (get stop-data "color") - "stop-opacity" (get stop-data "opacity")}})))) - - (data->gradient-def [id [color data]] - (let [id (str "gradient-" id "-" (subs color 1))] - (if (= type "linear") - {"type" "element" - "name" "linearGradient" - "attributes" {"id" id "x1" "0.5" "y1" "1" "x2" "0.5" "y2" "0"} - "elements" (get-stops data)} - - {"type" "element" - "name" "radialGradient" - "attributes" {"id" id "cx" "0.5" "cy" "0.5" "r" "0.5"} - "elements" (get-stops data)}))) - (get-gradients [id mapping] (->> mapping (filter (fn [[_color data]] (= (get data "type") "gradient"))) - (mapv (partial data->gradient-def id)))) + (mapv (partial svg-gradient/data->gradient-def id)))) (join-color-layers [{:keys [id x y width height mapping] :as node} layers] (l/trace :fn :join-color-layers :mapping mapping) @@ -369,4 +354,3 @@ (assoc :query (u/map->query-string params)))] (bw/exec! (prepare-options uri) (partial render uri))))) - diff --git a/exporter/src/app/renderer/svg_gradient.cljs b/exporter/src/app/renderer/svg_gradient.cljs new file mode 100644 index 0000000000..2efaca2c0d --- /dev/null +++ b/exporter/src/app/renderer/svg_gradient.cljs @@ -0,0 +1,32 @@ +;; 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 app.renderer.svg-gradient) + +(defn- get-stops + [data] + (->> (get-in data ["gradient" "stops"]) + (mapv (fn [stop-data] + {"type" "element" + "name" "stop" + "attributes" {"offset" (get stop-data "offset") + "stop-color" (get stop-data "color") + "stop-opacity" (get stop-data "opacity")}})))) + +(defn data->gradient-def + [id [color data]] + (let [id (str "gradient-" id "-" (subs color 1)) + gradient-type (get-in data ["gradient" "type"])] + (if (= gradient-type "linear") + {"type" "element" + "name" "linearGradient" + "attributes" {"id" id "x1" "0.5" "y1" "1" "x2" "0.5" "y2" "0"} + "elements" (get-stops data)} + + {"type" "element" + "name" "radialGradient" + "attributes" {"id" id "cx" "0.5" "cy" "0.5" "r" "0.5"} + "elements" (get-stops data)}))) diff --git a/exporter/src/app/util/shell.cljs b/exporter/src/app/util/shell.cljs index 60dc1bd6b8..8331888b00 100644 --- a/exporter/src/app/util/shell.cljs +++ b/exporter/src/app/util/shell.cljs @@ -94,14 +94,14 @@ (.readFile fs/promises fpath)) (defn run-cmd! - [cmd] + [cmd & args] (p/create (fn [resolve reject] - (l/trace :fn :run-cmd :cmd cmd) - (proc/exec cmd #js {:encoding "buffer"} - (fn [error stdout _stderr] - ;; (l/trace :fn :run-cmd :stdout stdout) - (if error - (reject error) - (resolve stdout))))))) + (l/trace :fn :run-cmd :cmd cmd :args args) + (proc/execFile cmd (clj->js args) #js {:encoding "buffer"} + (fn [error stdout _stderr] + ;; (l/trace :fn :run-cmd :stdout stdout) + (if error + (reject error) + (resolve stdout))))))) diff --git a/exporter/test/exporter_tests/renderer_svg_test.cljs b/exporter/test/exporter_tests/renderer_svg_test.cljs new file mode 100644 index 0000000000..d680b344ab --- /dev/null +++ b/exporter/test/exporter_tests/renderer_svg_test.cljs @@ -0,0 +1,25 @@ +;; 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 exporter-tests.renderer-svg-test + (:require + [app.renderer.svg-gradient :as svg-gradient] + [cljs.test :refer [deftest is testing]])) + +(def gradient-stops + [{"color" "#000000" "offset" 0 "opacity" 1} + {"color" "#ffffff" "offset" 1 "opacity" 1}]) + +(deftest creates-the-correct-gradient-element + (doseq [[gradient-type element-name] + [["linear" "linearGradient"] + ["radial" "radialGradient"]]] + (testing gradient-type + (let [gradient-data {"type" "gradient" + "gradient" {"type" gradient-type + "stops" gradient-stops}} + result (svg-gradient/data->gradient-def "text-id" ["#000001" gradient-data])] + (is (= element-name (get result "name"))))))) diff --git a/exporter/test/exporter_tests/runner.cljs b/exporter/test/exporter_tests/runner.cljs new file mode 100644 index 0000000000..a0aa4481e0 --- /dev/null +++ b/exporter/test/exporter_tests/runner.cljs @@ -0,0 +1,172 @@ +;; 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 exporter-tests.runner + (:require + [app.common.logging :as l] + [cljs.test :as t] + [clojure.string :as str] + [clojure.tools.cli :refer [parse-opts]] + [exporter-tests.renderer-svg-test] + [exporter-tests.shell-test] + [goog.object :as gobj])) + +(enable-console-print!) + +(def test-namespaces + ['exporter-tests.renderer-svg-test + 'exporter-tests.shell-test]) + +(assert (every? find-ns-obj test-namespaces) + "test-namespaces contains a namespace that isn't required in runner.cljs") + +(defmethod t/report [:cljs.test/default :begin-test-var] + [m] + (let [v (:var m)] + (println (str " ▸ " (:ns (meta v)) "/" (:name (meta v)))))) + +(defmethod t/report [:cljs.test/default :end-run-tests] + [result] + (.exit js/process (if (cljs.test/successful? result) 0 1))) + +(def ^:private log-levels + #{:trace :debug :info :warn :error}) + +(def cli-options + [["-f" "--focus FOCUS" "Run one test namespace or one test var, e.g. exporter-tests.renderer-svg-test/creates-the-correct-gradient-element"] + ["-l" "--log-level LEVEL" "Set app logger level: trace|debug|info|warn|error" + :parse-fn keyword + :validate [log-levels "must be one of trace, debug, info, warn, error"]] + ["-h" "--help"]]) + +(defn- argv + [] + (let [args (->> (.-argv js/process) + (array-seq) + (drop 2))] + ;; `pnpm run test -- --focus ...` forwards the separator to the node + ;; process, so drop one leading `--` before handing args to tools.cli. + (cond-> args + (= "--" (first args)) rest))) + +(defn- usage + [summary] + (str "Usage: node target/tests/test.js [options]\n\n" + "Options:\n" + summary "\n\n" + "Build first with: pnpm run build:test\n\n" + "Focus examples:\n" + " node target/tests/test.js --focus exporter-tests.renderer-svg-test\n" + " node target/tests/test.js --focus exporter-tests.renderer-svg-test/creates-the-correct-gradient-element\n\n" + "Log level example:\n" + " node target/tests/test.js --focus exporter-tests.renderer-svg-test --log-level warn")) + +(defn- fail! + [message] + (js/console.error message) + (.exit js/process 1)) + +(defn- parse-focus + [focus] + (let [[ns-name test-name & extra] (str/split focus #"/")] + (cond + (or (str/blank? ns-name) (seq extra)) + (fail! (str "Invalid --focus value: " focus)) + + (some? test-name) + {:ns (symbol ns-name) :test test-name} + + :else + {:ns (symbol ns-name)}))) + +(defn- fixture-value + [ns-obj fixture-name] + (let [value (gobj/get ns-obj (munge fixture-name))] + (when-not (undefined? value) + value))) + +(defn- ns-test-vars + [ns-sym] + (when-let [ns-obj (find-ns-obj ns-sym)] + (->> (js-keys ns-obj) + (keep (fn [key] + (some-> (gobj/get ns-obj key) + (.-cljs$lang$var)))) + (filter (comp :test meta)) + (sort-by (comp :line meta))))) + +(defn- ns-fixtures + [ns-sym vars] + (when-let [ns-obj (find-ns-obj ns-sym)] + (let [ns-key (or (some-> vars first meta :ns) ns-sym) + once-fixtures (fixture-value ns-obj "cljs-test-once-fixtures") + each-fixtures (fixture-value ns-obj "cljs-test-each-fixtures")] + {:once (when once-fixtures {ns-key once-fixtures}) + :each (when each-fixtures {ns-key each-fixtures})}))) + +(defn- selected-tests + [{:keys [ns test]}] + (when-not (some #{ns} test-namespaces) + (fail! (str "Unknown test namespace: " ns))) + (let [vars (vec (ns-test-vars ns))] + (when (empty? vars) + (fail! (str "No tests found in namespace: " ns))) + (if test + (let [test-sym (symbol test) + test-var (some #(when (= test-sym (:name (meta %))) %) vars)] + (if test-var + {:vars [test-var] + :fixtures (ns-fixtures ns [test-var])} + (fail! (str "Unknown test var: " ns "/" test)))) + {:vars vars + :fixtures (ns-fixtures ns vars)}))) + +(defn- merge-fixtures + [fixtures] + {:once (apply merge (keep :once fixtures)) + :each (apply merge (keep :each fixtures))}) + +(defn- run-test-vars! + [tests] + (let [vars (vec (mapcat :vars tests)) + fixtures (merge-fixtures (map :fixtures tests)) + env (assoc (t/empty-env) + :once-fixtures (:once fixtures) + :each-fixtures (:each fixtures)) + summary (volatile! {:test 0 :pass 0 :fail 0 :error 0 :type :summary})] + (t/set-env! env) + (t/run-block + (concat (t/test-vars-block vars) + [(fn [] + (vswap! summary + (partial merge-with +) + (:report-counters (t/get-current-env)))) + (fn [] + (t/report @summary) + (t/report (assoc @summary :type :end-run-tests)))])))) + +(defn- run-focused-test! + [focus] + (run-test-vars! [(selected-tests (parse-focus focus))])) + +(defn -main + [] + (let [{:keys [options errors summary]} (parse-opts (argv) cli-options)] + (cond + (seq errors) + (fail! (str/join "\n" errors)) + + (:help options) + (do + (println (usage summary)) + (.exit js/process 0)) + + :else + (do + (l/setup! {:app (or (:log-level options) :warn)}) + (if (:focus options) + (run-focused-test! (:focus options)) + (run-test-vars! (map #(selected-tests {:ns %}) test-namespaces))))))) diff --git a/exporter/test/exporter_tests/shell_test.cljs b/exporter/test/exporter_tests/shell_test.cljs new file mode 100644 index 0000000000..e232a4ed67 --- /dev/null +++ b/exporter/test/exporter_tests/shell_test.cljs @@ -0,0 +1,70 @@ +;; 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 exporter-tests.shell-test + "Tests to verify GHSA-4f36-m4hj-cv86 is fixed: OS Command Injection in SVG exporter. + These tests prove that: + 1. execFile does NOT interpret shell metacharacters (safe execution) + 2. Malicious colors fail validation regex + 3. The injection does NOT execute commands (no RCE)" + (:require + ["node:child_process" :as proc] + ["node:fs" :as fs] + [cljs.test :as t :include-macros true])) + +(def ^:private hex-color-rx + #"^#(?:[0-9a-fA-F]{3}){1,2}$") + +(defn- valid-hex-color? + [color] + (and (string? color) + (some? (re-matches hex-color-rx color)))) + +(t/deftest execfile-does-not-interpret-shell-metacharacters + (t/testing "Proves execFile passes arguments literally (no shell interpretation)" + (t/async done + (let [cmd "echo" + args #js ["$(echo PWNED)"]] + (proc/execFile cmd args #js {:encoding "buffer"} + (fn [error stdout _stderr] + (if error + (do + (t/is false (str "unexpected error: " (.-message error))) + (done)) + (let [output (.toString stdout "utf8")] + (t/is (= "$(echo PWNED)\n" output) + "execFile passes $(...) literally, no shell interpretation") + (done))))))))) + +(t/deftest malicious-color-fails-validation + (t/testing "Proves malicious colors are rejected by validation" + (let [malicious "#000000$(echo PWNED)" + valid-color "#000000" + short-valid "#abc"] + (t/is (not (valid-hex-color? malicious)) + "malicious color with $(...) fails validation") + (t/is (valid-hex-color? valid-color) + "valid 6-digit hex color passes validation") + (t/is (valid-hex-color? short-valid) + "valid 3-digit hex color passes validation")))) + +(t/deftest execfile-does-not-execute-injected-commands + (t/testing "Proves execFile does NOT execute injected commands (no RCE)" + (t/async done + (let [marker "/tmp/penpot-exporter-rce-test" + malicious (str "#000000$(touch " marker ")") + cmd "echo" + args #js [malicious]] + (when (fs/existsSync marker) + (fs/unlinkSync marker)) + (proc/execFile cmd args #js {:encoding "buffer"} + (fn [_error _stdout _stderr] + ;; Command completes (or fails), but no injection occurs + (t/is (not (fs/existsSync marker)) + "no RCE: marker file was NOT created") + (when (fs/existsSync marker) + (fs/unlinkSync marker)) + (done))))))) diff --git a/frontend/src/app/main/ui/shapes/text/fo_text.cljs b/frontend/src/app/main/ui/shapes/text/fo_text.cljs index 5bb224b673..9a78b482e3 100644 --- a/frontend/src/app/main/ui/shapes/text/fo_text.cljs +++ b/frontend/src/app/main/ui/shapes/text/fo_text.cljs @@ -79,15 +79,21 @@ {:type :gradient :gradient fill-color-gradient} - (and (string? fill-color) (some? fill-opacity) (not= fill-opacity 1)) + (and (string? fill-color) + (cc/hex-color-string? fill-color) + (some? fill-opacity) + (not= fill-opacity 1)) {:type :transparent :hex fill-color :opacity fill-opacity} - (string? fill-color) + (and (string? fill-color) + (cc/hex-color-string? fill-color)) {:type :solid :hex fill-color - :map-to fill-color})) + :map-to fill-color} + + :else nil)) (defn- retrieve-colors "Given a text shape returns a triple with the values: diff --git a/scripts/ci b/scripts/ci index 026bdb7d02..f78bbd0795 100755 --- a/scripts/ci +++ b/scripts/ci @@ -26,7 +26,7 @@ declare -A LINT_CMD=( [backend]="pnpm run lint:clj" [common]="pnpm run lint:clj" [render-wasm]="./lint" - [exporter]="pnpm run lint" + [exporter]="pnpm run lint:clj" [mcp]="" [plugins]="pnpm run lint" [library]="pnpm run lint" @@ -37,7 +37,7 @@ declare -A TEST_CMD=( [backend]="clojure -M:dev:test" [common]="clojure -M:dev:test && pnpm run test:quiet" [render-wasm]="./test" - [exporter]="" + [exporter]="pnpm run test:quiet" [mcp]="pnpm run test" [plugins]="pnpm run test" [library]="pnpm run test" @@ -48,7 +48,7 @@ declare -A FMT_CHECK_CMD=( [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" + [exporter]="pnpm run check-fmt:clj" [mcp]="pnpm run fmt:check" [plugins]="pnpm run format:check" [library]="pnpm run check-fmt" @@ -59,7 +59,7 @@ declare -A FMT_FIX_CMD=( [backend]="pnpm run fmt" [common]="pnpm run fmt:clj && pnpm run fmt:js" [render-wasm]="cargo fmt" - [exporter]="pnpm run fmt" + [exporter]="pnpm run fmt:clj" [mcp]="pnpm run fmt" [plugins]="pnpm run format" [library]="pnpm run fmt" @@ -70,7 +70,7 @@ declare -A PAREN_REPAIR_CMD=( [backend]="find src test -name '*.clj' -o -name '*.cljs' -o -name '*.cljc' | xargs $PROJECT_ROOT/scripts/paren-repair" [common]="find src test -name '*.clj' -o -name '*.cljs' -o -name '*.cljc' | xargs $PROJECT_ROOT/scripts/paren-repair" [render-wasm]="" - [exporter]="find src -name '*.clj' -o -name '*.cljs' -o -name '*.cljc' | xargs $PROJECT_ROOT/scripts/paren-repair" + [exporter]="find src test -name '*.clj' -o -name '*.cljs' -o -name '*.cljc' | xargs $PROJECT_ROOT/scripts/paren-repair" [mcp]="" [plugins]="" [library]="find src test -name '*.clj' -o -name '*.cljs' -o -name '*.cljc' | xargs $PROJECT_ROOT/scripts/paren-repair" From c378ec9218c3e3845528a7bcc62dfa6ca32ef87e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marina=20L=C3=B3pez?= Date: Wed, 19 Aug 2026 14:23:40 +0200 Subject: [PATCH 16/19] :bug: Avoid swallowing fatal errors in organization sso telemetry (#11279) --- backend/src/app/auth/oidc.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 6cb23fb9de..164edafd76 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -845,7 +845,7 @@ cfg request (some-> (session/get-session request) :profile-id) (:organization-id state) "organization-sso-auth-failed" :failure-reason (organization-sso-oauth-failure-reason error)))) - (catch Throwable _ nil))) + (catch Exception _ nil))) (defn- non-blank-uri [value] From a91c796b0e4ea5c282eab94ccfdde1b456e0ba2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elena=20Torr=C3=B3?= Date: Wed, 19 Aug 2026 16:19:15 +0200 Subject: [PATCH 17/19] :bug: Fix missing zip export on tempfile types (#11292) --- common/src/app/common/media.cljc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/src/app/common/media.cljc b/common/src/app/common/media.cljc index 3d67bc75b6..a5a74e6c75 100644 --- a/common/src/app/common/media.cljc +++ b/common/src/app/common/media.cljc @@ -23,7 +23,7 @@ "image/svg+xml"}) (def tempfile-types - (conj image-types "application/pdf")) + (conj image-types "application/pdf" "application/zip")) (defn format->extension [format] From c200a4d777c04f3f9a745728d8fc02a7219de274 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 18:21:55 +0200 Subject: [PATCH 18/19] :bug: Fix HTML escaping in notification pill detail section (#11275) The notification pill component now properly respects the `is-html` flag when rendering the detail section, matching the behavior of the children section. Token import error messages now escape HTML characters in user-provided values like token names and type names before displaying them in notifications. AI-assisted-by: qwen3.7-plus --- frontend/src/app/main/data/workspace/tokens/errors.cljs | 5 +++-- .../src/app/main/data/workspace/tokens/import_export.cljs | 6 ++++-- .../main/ui/ds/notifications/shared/notification_pill.cljs | 6 ++++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/frontend/src/app/main/data/workspace/tokens/errors.cljs b/frontend/src/app/main/data/workspace/tokens/errors.cljs index e5716f07be..8ca54af835 100644 --- a/frontend/src/app/main/data/workspace/tokens/errors.cljs +++ b/frontend/src/app/main/data/workspace/tokens/errors.cljs @@ -6,6 +6,7 @@ (ns app.main.data.workspace.tokens.errors (:require + [app.util.dom :as dom] [app.util.i18n :refer [tr]] [cuerdas.core :as str])) @@ -25,12 +26,12 @@ :error.import/invalid-token-name {:error/code :error.import/invalid-token-name :error/fn #(tr "errors.tokens.invalid-json-token-name") - :error/detail #(tr "errors.tokens.invalid-json-token-name-detail" %)} + :error/detail #(tr "errors.tokens.invalid-json-token-name-detail" (dom/escape-html %))} :error.import/style-dictionary-reference-errors {:error/code :error.import/style-dictionary-reference-errors :error/fn #(str (tr "errors.tokens.import-error") "\n\n" (first %)) - :error/detail #(str/join "\n\n" (rest %))} + :error/detail #(str/join "\n\n" (map dom/escape-html (rest %)))} :error.import/style-dictionary-unknown-error {:error/code :error.import/style-dictionary-reference-errors diff --git a/frontend/src/app/main/data/workspace/tokens/import_export.cljs b/frontend/src/app/main/data/workspace/tokens/import_export.cljs index 5dded5bbb3..5c7c9151b7 100644 --- a/frontend/src/app/main/data/workspace/tokens/import_export.cljs +++ b/frontend/src/app/main/data/workspace/tokens/import_export.cljs @@ -16,6 +16,7 @@ [app.main.data.tokenscript :as ts] [app.main.data.workspace.tokens.errors :as wte] [app.main.store :as st] + [app.util.dom :as dom] [app.util.i18n :as i18n] [beicon.v2.core :as rx] [cuerdas.core :as str])) @@ -54,14 +55,15 @@ (l/wrn :hint "unsupported token types found during import" :tokens (str/join ", " (map (fn [[path type]] (str path " (" type ")")) unknown-tokens))) (ntf/show {:content (i18n/tr "workspace.tokens.unknown-token-type-message") + :is-html true :detail (->> (for [[token-type token-paths] type->tokens] (str (i18n/tr "workspace.tokens.unknown-token-type-section" - token-type + (dom/escape-html token-type) (i18n/tr "labels.warning-count" (i18n/c (count token-paths)))) "
    " (->> token-paths (sort) - (map #(str "
  • " % "
    • ")) + (map #(str "
  • " (dom/escape-html %) "
    • ")) (str/join "")) "
")) (str/join "")) diff --git a/frontend/src/app/main/ui/ds/notifications/shared/notification_pill.cljs b/frontend/src/app/main/ui/ds/notifications/shared/notification_pill.cljs index 0a774f33ac..c14696c988 100644 --- a/frontend/src/app/main/ui/ds/notifications/shared/notification_pill.cljs +++ b/frontend/src/app/main/ui/ds/notifications/shared/notification_pill.cljs @@ -59,5 +59,7 @@ (when detail [:details {:class (stl/css :error-detail)} [:summary {:class (stl/css :error-detail-summary)} (tr "workspace.notification-pill.detail")] - [:div {:class (stl/css :error-detail-content) - :dangerouslySetInnerHTML #js {:__html detail}}]])])) + (if is-html + [:div {:class (stl/css :error-detail-content) + :dangerouslySetInnerHTML #js {:__html detail}}] + [:div {:class (stl/css :error-detail-content)} detail])])])) From 209aea83658f209c4189b531eeda0f3a638a0294 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 18:26:35 +0200 Subject: [PATCH 19/19] :bug: Add proper ownership check on managing/deleting shared link on a file (#11290) * :bug: Add ownership check to share-link deletion The delete-share-link RPC command only verified file-level edit permission but did not check if the caller owned the share-link. This allowed any file editor to delete share-links created by other users, disrupting collaborative workflows. The fix adds an ownership check that allows deletion only by: - The share-link creator (owner-id matches profile-id) - File admins (is-admin permission) - File owners (is-owner permission) Implemented using TDD: - RED: Test demonstrates IDOR vulnerability (editor can delete) - GREEN: Ownership check prevents unauthorized deletion - All existing tests continue to pass Closes #11289 AI-assisted-by: qwen3.7-plus * :bug: Add test coverage for share-link deletion escape hatches Address code review feedback for PR #11290: - Add test for editor deleting their own share-link - Add test for admin deleting editor's share-link - Add test for owner deleting editor's share-link - Remove redundant :is-owner check (already included in :is-admin) - Add clarifying comment about :is-admin including :is-owner Closes #11289 AI-assisted-by: qwen3.7-plus --- backend/src/app/rpc/commands/files_share.clj | 13 ++ backend/test/backend_tests/rpc_file_test.clj | 127 +++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/backend/src/app/rpc/commands/files_share.clj b/backend/src/app/rpc/commands/files_share.clj index 9a8326d06d..0e8c184ede 100644 --- a/backend/src/app/rpc/commands/files_share.clj +++ b/backend/src/app/rpc/commands/files_share.clj @@ -7,6 +7,8 @@ (ns app.rpc.commands.files-share "Share link related rpc mutation methods." (:require + [app.binfile.common :as bfc] + [app.common.exceptions :as ex] [app.common.schema :as sm] [app.common.uuid :as uuid] [app.db :as db] @@ -66,5 +68,16 @@ [{:keys [::db/conn]} {:keys [::rpc/profile-id id] :as params}] (let [slink (db/get-by-id conn :share-link id)] (files/check-edition-permissions! conn profile-id (:file-id slink)) + + ;; Verify caller owns this specific share-link, OR has admin access. + ;; Note: :is-admin already includes :is-owner (see bfc/get-file-permissions), + ;; so we only need to check :is-admin here. + (let [perms (bfc/get-file-permissions conn profile-id (:file-id slink))] + (when-not (or (= (:owner-id slink) profile-id) + (:is-admin perms)) + (ex/raise :type :authorization + :code :not-share-link-owner + :hint "You can only delete share-links you created"))) + (db/delete! conn :share-link {:id id}) nil)) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index d1ec0eb233..27e881dc45 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -2467,3 +2467,130 @@ err (:error out)] (t/is (th/ex-info? err)) (t/is (th/ex-of-type? err :not-found)))) + +(t/deftest share-link-deletion-idor + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + admin (th/create-profile* 3 {:is-active true}) + proj-id (:default-project-id owner) + team-id (:default-team-id owner) + + file (th/create-file* 1 {:profile-id (:id owner) + :project-id proj-id + :is-shared false}) + + ;; Invite editor to the team with edit permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id editor) + :role :editor}) + + ;; Invite admin to the team with admin permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id admin) + :role :admin}) + + ;; Owner creates a share-link + slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{(get-in file [:data :pages 0])} + :who-comment "team" + :who-inspect "all"}) + slink-id (get-in slink [:result :id])] + + (t/testing "owner can delete their own share-link" + (let [out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id owner) + :id slink-id})] + (t/is (nil? (:error out))))) + + (t/testing "editor CANNOT delete owner's share-link (IDOR)" + ;; Recreate the share-link for this test + (let [slink2 (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink2-id (get-in slink2 [:result :id]) + + ;; Editor tries to delete owner's share-link + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id editor) + :id slink2-id}) + err (:error out) + edata (ex-data err)] + + ;; Should be denied with authorization error + (t/is (th/ex-info? err)) + (t/is (= :authorization (:type edata))) + + ;; Verify the share-link still exists + (let [check (th/command! {::th/type :get-view-only-bundle + ::rpc/profile-id (:id owner) + :file-id (:id file)}) + share-links (:share-links (:result check))] + (t/is (some #(= slink2-id (:id %)) share-links))))))) + +(t/deftest share-link-deletion-escape-hatches + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + admin (th/create-profile* 3 {:is-active true}) + proj-id (:default-project-id owner) + team-id (:default-team-id owner) + + file (th/create-file* 1 {:profile-id (:id owner) + :project-id proj-id + :is-shared false}) + + ;; Invite editor to the team with edit permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id editor) + :role :editor}) + + ;; Invite admin to the team with admin permissions + _ (th/create-team-role* {:team-id team-id + :profile-id (:id admin) + :role :admin})] + + (t/testing "editor CAN delete their own share-link" + (let [slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id editor) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink-id (get-in slink [:result :id]) + + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id editor) + :id slink-id})] + (t/is (nil? (:error out))))) + + (t/testing "admin CAN delete editor's share-link" + (let [slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id editor) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink-id (get-in slink [:result :id]) + + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id admin) + :id slink-id})] + (t/is (nil? (:error out))))) + + (t/testing "owner CAN delete editor's share-link" + (let [slink (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id editor) + :file-id (:id file) + :pages #{} + :who-comment "team" + :who-inspect "team"}) + slink-id (get-in slink [:result :id]) + + out (th/command! {::th/type :delete-share-link + ::rpc/profile-id (:id owner) + :id slink-id})] + (t/is (nil? (:error out)))))))