diff --git a/.serena/memories/workflow/creating-prs.md b/.serena/memories/workflow/creating-prs.md index c7a449fea2..8b3f8e3b20 100644 --- a/.serena/memories/workflow/creating-prs.md +++ b/.serena/memories/workflow/creating-prs.md @@ -30,7 +30,7 @@ See `mem:workflow/creating-commits` for emoji codes. Squash merge uses the PR ti Include concise sections covering: - what changed and why; -- related GitHub issues or Taiga stories (`Fixes #NNNN`, `Relates to #NNNN`, `Taiga #NNNN`); +- related GitHub issues or Taiga stories (`Closes #NNNN`, `Relates to #NNNN`, `Taiga #NNNN`); - screenshots or recordings for UI-visible changes; - testing performed and residual risk; - breaking changes or migration notes, if any. diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 81b1ef13ec..fa2faa8a55 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -65,12 +65,25 @@ :else request))) + ;; The specific-exception branches below (IAE, + ;; RequestTooBigException, EOFException) raise with + ;; `ex/raise` rather than calling `errors/handle` directly. + ;; This is intentional: the throw is caught by the + ;; top-level error handler in `app.http/router-handler` + ;; (`backend/src/app/http.clj`), which routes every + ;; uncaught exception through `errors/handle`. The + ;; per-route `wrap-errors` middleware in the route list + ;; is a defensive layer; correctness does not depend on + ;; it. Raising here keeps the cond uniform with the + ;; existing RequestTooBigException / EOFException + ;; branches. (handle-error [cause request] (cond - (instance? RuntimeException cause) - (if-let [cause (ex-cause cause)] - (handle-error cause request) - (errors/handle cause request)) + (instance? IllegalArgumentException cause) + (ex/raise :type :validation + :code :malformed-json + :hint (ex-message cause) + :cause cause) (instance? RequestTooBigException cause) (ex/raise :type :validation @@ -83,6 +96,11 @@ :hint (ex-message cause) :cause cause) + (instance? RuntimeException cause) + (if-let [cause (ex-cause cause)] + (handle-error cause request) + (errors/handle cause request)) + :else (errors/handle cause request)))] diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index f751d8aca6..bd986fc031 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -21,19 +21,73 @@ [clojure.test :as t] [mockery.core :refer [with-mocks]] [yetti.request :as yreq] - [yetti.response :as yres])) + [yetti.response :as yres]) + (:import + io.undertow.server.RequestTooBigException)) (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) -(defrecord DummyRequest [headers cookies] +(defrecord DummyRequest [headers cookies method body-stream + remote-addr server-name server-port + scheme protocol path query ssl-client-cert] yreq/IRequestCookies (get-cookie [_ name] {:value (get cookies name)}) yreq/IRequest (get-header [_ name] - (get headers name))) + (get headers name)) + (method [_] method) + (body [_] body-stream) + (path [_] path) + (query [_] query) + (server-port [_] server-port) + (server-name [_] server-name) + (remote-addr [_] remote-addr) + (ssl-client-cert [_] ssl-client-cert) + (scheme [_] scheme) + (protocol [_] protocol)) + +(defn- make-dummy-request + "Constructs a DummyRequest from an options map. Every key is + optional; missing values fall back to sensible defaults. New + fields added to DummyRequest won't break existing call sites + as long as this constructor keeps its `:or` defaults in sync. + + Recognized keys: + :headers — map of header name → value + :cookies — map of cookie name → value + :method — HTTP method keyword (default :get) + :body-stream — InputStream for the body (used directly) + :body-bytes — bytes or string for the body; wrapped in a + ByteArrayInputStream if :body-stream is not + given + :remote-addr — string (default \"127.0.0.1\") + :server-name — string (default \"test\") + :server-port — long (default 0) + :scheme — keyword (default :http) + :protocol — string (default \"HTTP/1.1\") + :path — string (default \"/test\") + :query — string or nil (default nil) + :ssl-client-cert — X509Certificate or nil (default nil)" + [{:keys [headers cookies method body-stream body-bytes + remote-addr server-name server-port scheme protocol + path query ssl-client-cert] + :or {headers {} cookies {} method :get + body-stream nil + remote-addr "127.0.0.1" server-name "test" server-port 0 + scheme :http protocol "HTTP/1.1" path "/test" query nil + ssl-client-cert nil}}] + (let [body-stream (or body-stream + (when body-bytes + (java.io.ByteArrayInputStream. + (if (string? body-bytes) + (.getBytes ^String body-bytes "UTF-8") + body-bytes))))] + (->DummyRequest headers cookies method body-stream + remote-addr server-name server-port + scheme protocol path query ssl-client-cert))) (t/deftest auth-middleware-1 (let [request (volatile! nil) @@ -41,11 +95,11 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {"authorization" "Token aaaa"} {})) + (handler (make-dummy-request {:headers {"authorization" "Token aaaa"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :token token-type)) @@ -58,10 +112,10 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {"authorization" "Bearer aaaa"} {})) + (handler (make-dummy-request {:headers {"authorization" "Bearer aaaa"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :bearer token-type)) @@ -74,10 +128,10 @@ (fn [req] (vreset! request req)) {})] - (handler (->DummyRequest {} {})) + (handler (make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (->DummyRequest {} {"auth-token" "foobar"})) + (handler (make-dummy-request {:cookies {"auth-token" "foobar"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :cookie token-type)) @@ -89,16 +143,16 @@ (fn [req] {::yres/status 200}) {:test1 "secret-key"})] - (let [response (handler (->DummyRequest {} {}))] + (let [response (handler (make-dummy-request {}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "secret-key2"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "secret-key"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (->DummyRequest {"x-shared-key" "test1 secret-key"} {}))] + (let [response (handler (make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))] (t/is (= 200 (::yres/status response)))))) (t/deftest access-token-authz @@ -209,7 +263,7 @@ :user-agent "user agent"}) (#'session/assign-token cfg)) - response (handler (->DummyRequest {} {"auth-token" (:token session)})) + response (handler (make-dummy-request {:cookies {"auth-token" (:token session)}})) {:keys [token claims] token-type :type} (get response ::http/auth-data)] @@ -220,3 +274,127 @@ (t/is (= "penpot" (:aud claims))) (t/is (= (:id session) (:sid claims))) (t/is (= (:id profile) (:uid claims))))) + +(t/deftest parse-request-illegal-argument-exception + ;; clojure.data.json raises IllegalArgumentException (case + ;; fall-through) on several kinds of malformed input. The + ;; parse-request middleware should convert any such IAE into a + ;; 400 :malformed-json validation error rather than letting it + ;; surface as a 500 internal error. Because the conversion is + ;; done by raising an ex-info (caught by the top-level error + ;; handler in app.http/router-handler), this test asserts on + ;; the ex-info thrown by wrap-parse-request directly. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] {::yres/status 200 ::yres/body :ok})) + ;; Body contains the bytes for: {"x": "\}"} -- a string + ;; value with a backslash followed by '}', which + ;; clojure.data.json v0.5.x cannot handle. + body (.getBytes "{\"x\": \"\\}\"}" "UTF-8") + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes body}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))) + (t/is (string? (-> ex ex-data :hint))))) + +(t/deftest parse-request-request-too-big-exception + ;; When RequestTooBigException is raised (e.g. the request body + ;; exceeded the configured size limit), the middleware should + ;; convert it to a 413 :request-body-too-large validation + ;; error. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (RequestTooBigException. "too large")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :request-body-too-large (-> ex ex-data :code))) + (t/is (string? (-> ex ex-data :hint))))) + +(t/deftest parse-request-eof-exception + ;; When java.io.EOFException is raised (e.g. the body stream + ;; was closed before the parser could read it), the middleware + ;; should convert it to a 400 :malformed-json validation error. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (java.io.EOFException. "stream closed")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))) + (t/is (string? (-> ex ex-data :hint))))) + +(t/deftest parse-request-runtime-exception-with-cause + ;; When a RuntimeException with a non-nil ex-cause is raised, + ;; the middleware should recurse on the cause and dispatch + ;; through the specific-exception branches. Here we wrap an + ;; IllegalArgumentException in a RuntimeException and verify + ;; it surfaces as :malformed-json. + (let [iae (IllegalArgumentException. "No matching clause: 99") + wrapped (doto (RuntimeException. "wrapped") + (.initCause iae)) + handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw wrapped))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + ex (try + (handler request) + (catch clojure.lang.ExceptionInfo e e))] + (t/is (instance? clojure.lang.ExceptionInfo ex)) + (t/is (= :validation (-> ex ex-data :type))) + (t/is (= :malformed-json (-> ex ex-data :code))))) + +(t/deftest parse-request-runtime-exception-without-cause + ;; When a bare RuntimeException (no ex-cause) is raised, the + ;; middleware should fall through to errors/handle's :default + ;; path and return a 500 with :type :server-error :code + ;; :unexpected. This is the "true internal error" path. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (RuntimeException. "boom")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + response (handler request) + body (::yres/body response)] + (t/is (= 500 (::yres/status response))) + (t/is (= :server-error (:type body))) + (t/is (= :unexpected (:code body))) + (t/is (= "boom" (:hint body))))) + +(t/deftest parse-request-non-runtime-throwable + ;; When a non-RuntimeException Throwable is raised (e.g. an + ;; Error subclass or a non-RuntimeException checked-style + ;; exception), the middleware should fall through to the + ;; :else branch and call errors/handle. java.io.IOException + ;; has a dedicated handle-exception method that returns 500 + ;; with :code :io-exception. + (let [handler (#'app.http.middleware/wrap-parse-request + (fn [_] (throw (java.io.IOException. "network gone")))) + request (make-dummy-request + {:method :post + :headers {"content-type" "application/json"} + :body-bytes (.getBytes "{}" "UTF-8")}) + response (handler request) + body (::yres/body response)] + (t/is (= 500 (::yres/status response))) + (t/is (= :server-error (:type body))) + (t/is (= :io-exception (:code body))) + (t/is (= "network gone" (:hint body))))) diff --git a/common/src/app/common/geom/shapes/flex_layout/bounds.cljc b/common/src/app/common/geom/shapes/flex_layout/bounds.cljc index e6aef398f0..b434d99fc9 100644 --- a/common/src/app/common/geom/shapes/flex_layout/bounds.cljc +++ b/common/src/app/common/geom/shapes/flex_layout/bounds.cljc @@ -121,74 +121,79 @@ (defn layout-content-points [bounds parent children objects] - (let [parent-id (dm/get-prop parent :id) - parent-bounds @(get bounds parent-id) - reverse? (ctl/reverse? parent) - children (cond->> children (not reverse?) reverse)] + (let [parent-id (dm/get-prop parent :id) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [reverse? (ctl/reverse? parent) + children (cond->> children (not reverse?) reverse)] - (loop [children (seq children) - result (transient []) - correct-v (gpt/point 0)] + (loop [children (seq children) + result (transient []) + correct-v (gpt/point 0)] - (if (not children) - (persistent! result) + (if (not children) + (persistent! result) - (let [child (first children) - child-id (dm/get-prop child :id) - child-bounds @(get bounds child-id) - [margin-top margin-right margin-bottom margin-left] (ctl/child-margins child) + (let [child (first children) + child-id (dm/get-prop child :id) + child-bounds-ref (get bounds child-id) + child-bounds (some-> child-bounds-ref deref) + [margin-top margin-right margin-bottom margin-left] (ctl/child-margins child) - [child-bounds correct-v] - (if (or (ctl/fill-width? child) (ctl/fill-height? child)) - (child-layout-bound-points parent child parent-bounds child-bounds correct-v bounds objects) - [(->> child-bounds (map #(gpt/add % correct-v))) correct-v]) + [child-bounds correct-v] + (if (and child-bounds + (or (ctl/fill-width? child) (ctl/fill-height? child))) + (child-layout-bound-points parent child parent-bounds child-bounds correct-v bounds objects) + [(when child-bounds + (->> child-bounds (map #(gpt/add % correct-v)))) + correct-v]) - child-bounds - (when (d/not-empty? child-bounds) - (-> (gpo/parent-coords-bounds child-bounds parent-bounds) - (gpo/pad-points (- margin-top) (- margin-right) (- margin-bottom) (- margin-left))))] + child-bounds + (when (d/not-empty? child-bounds) + (-> (gpo/parent-coords-bounds child-bounds parent-bounds) + (gpo/pad-points (- margin-top) (- margin-right) (- margin-bottom) (- margin-left))))] - (recur (next children) - (cond-> result (some? child-bounds) (conj! child-bounds)) - correct-v)))))) + (recur (next children) + (cond-> result (some? child-bounds) (conj! child-bounds)) + correct-v)))))))) (defn layout-content-bounds [bounds {:keys [layout-padding] :as parent} children objects] - (let [parent-id (:id parent) - parent-bounds @(get bounds parent-id) + (let [parent-id (:id parent) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [row? (ctl/row? parent) + col? (ctl/col? parent) + space-around? (ctl/space-around? parent) + space-evenly? (ctl/space-evenly? parent) + content-evenly? (ctl/content-evenly? parent) + [layout-gap-row layout-gap-col] (ctl/gaps parent) - row? (ctl/row? parent) - col? (ctl/col? parent) - space-around? (ctl/space-around? parent) - space-evenly? (ctl/space-evenly? parent) - content-evenly? (ctl/content-evenly? parent) - [layout-gap-row layout-gap-col] (ctl/gaps parent) + row-pad (if (or (and col? space-evenly?) + (and col? space-around?) + (and row? content-evenly?)) + layout-gap-row + 0) - row-pad (if (or (and col? space-evenly?) - (and col? space-around?) - (and row? content-evenly?)) - layout-gap-row - 0) + col-pad (if (or (and row? space-evenly?) + (and row? space-around?) + (and col? content-evenly?)) + layout-gap-col + 0) - col-pad (if (or (and row? space-evenly?) - (and row? space-around?) - (and col? content-evenly?)) - layout-gap-col - 0) + {pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding + pad-top (+ (or pad-top 0) row-pad) + pad-right (+ (or pad-right 0) col-pad) + pad-bottom (+ (or pad-bottom 0) row-pad) + pad-left (+ (or pad-left 0) col-pad) - {pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding - pad-top (+ (or pad-top 0) row-pad) - pad-right (+ (or pad-right 0) col-pad) - pad-bottom (+ (or pad-bottom 0) row-pad) - pad-left (+ (or pad-left 0) col-pad) + layout-points + (layout-content-points bounds parent children objects)] - layout-points - (layout-content-points bounds parent children objects)] - - (if (d/not-empty? layout-points) - (-> layout-points - (gpo/merge-parent-coords-bounds parent-bounds) - (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) - ;; Cannot create some bounds from the children so we return the parent's - parent-bounds))) + (if (d/not-empty? layout-points) + (-> layout-points + (gpo/merge-parent-coords-bounds parent-bounds) + (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) + ;; Cannot create some bounds from the children so we return the parent's + parent-bounds))))) diff --git a/common/src/app/common/geom/shapes/grid_layout/bounds.cljc b/common/src/app/common/geom/shapes/grid_layout/bounds.cljc index 0628cf2060..caadff4766 100644 --- a/common/src/app/common/geom/shapes/grid_layout/bounds.cljc +++ b/common/src/app/common/geom/shapes/grid_layout/bounds.cljc @@ -12,36 +12,36 @@ (defn layout-content-points [bounds parent {:keys [row-tracks column-tracks]}] - (let [parent-id (:id parent) - parent-bounds @(get bounds parent-id) - - hv #(gpo/start-hv parent-bounds %) - vv #(gpo/start-vv parent-bounds %)] - (d/concat-vec - (->> row-tracks - (mapcat #(vector (:start-p %) - (gpt/add (:start-p %) (vv (:size %)))))) - (->> column-tracks - (mapcat #(vector (:start-p %) - (gpt/add (:start-p %) (hv (:size %))))))))) + (let [parent-id (:id parent) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [hv #(gpo/start-hv parent-bounds %) + vv #(gpo/start-vv parent-bounds %)] + (d/concat-vec + (->> row-tracks + (mapcat #(vector (:start-p %) + (gpt/add (:start-p %) (vv (:size %)))))) + (->> column-tracks + (mapcat #(vector (:start-p %) + (gpt/add (:start-p %) (hv (:size %))))))))))) (defn layout-content-bounds [bounds {:keys [layout-padding] :as parent} layout-data] - (let [parent-id (:id parent) - parent-bounds @(get bounds parent-id) + (let [parent-id (:id parent) + parent-bounds (get bounds parent-id)] + (when-let [parent-bounds (some-> parent-bounds deref)] + (let [{pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding + pad-top (or pad-top 0) + pad-right (or pad-right 0) + pad-bottom (or pad-bottom 0) + pad-left (or pad-left 0) - {pad-top :p1 pad-right :p2 pad-bottom :p3 pad-left :p4} layout-padding - pad-top (or pad-top 0) - pad-right (or pad-right 0) - pad-bottom (or pad-bottom 0) - pad-left (or pad-left 0) + layout-points (layout-content-points bounds parent layout-data)] - layout-points (layout-content-points bounds parent layout-data)] - - (if (d/not-empty? layout-points) - (-> layout-points - (gpo/merge-parent-coords-bounds parent-bounds) - (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) - ;; Cannot create some bounds from the children so we return the parent's - parent-bounds))) + (if (d/not-empty? layout-points) + (-> layout-points + (gpo/merge-parent-coords-bounds parent-bounds) + (gpo/pad-points (- pad-top) (- pad-right) (- pad-bottom) (- pad-left))) + ;; Cannot create some bounds from the children so we return the parent's + parent-bounds))))) diff --git a/common/src/app/common/geom/shapes/min_size_layout.cljc b/common/src/app/common/geom/shapes/min_size_layout.cljc index 89f17871e3..57375098e9 100644 --- a/common/src/app/common/geom/shapes/min_size_layout.cljc +++ b/common/src/app/common/geom/shapes/min_size_layout.cljc @@ -31,13 +31,17 @@ (and (ctl/fill-width? child) (ctl/grid-layout? child)) - (let [children - (->> (cfh/get-immediate-children objects (:id child)) - (remove ctl/position-absolute?) - (map #(vector @(get bounds (:id %)) %))) - layout-data (gd/calc-layout-data child @(get bounds (:id child)) children bounds objects true)] - (max (ctl/child-min-width child) - (gpo/width-points (gb/layout-content-bounds bounds child layout-data)))) + (let [child-bounds-ref (get bounds (:id child))] + (if child-bounds-ref + (let [children + (->> (cfh/get-immediate-children objects (:id child)) + (remove ctl/position-absolute?) + (keep #(when-let [b (get bounds (:id %))] + [@b %]))) + layout-data (gd/calc-layout-data child @child-bounds-ref children bounds objects true)] + (max (ctl/child-min-width child) + (gpo/width-points (gb/layout-content-bounds bounds child layout-data)))) + (ctl/child-min-width child))) (ctl/fill-width? child) (ctl/child-min-width child) @@ -63,11 +67,15 @@ (let [children (->> (cfh/get-immediate-children objects (dm/get-prop child :id)) (remove ctl/position-absolute?) - (map (fn [child] [@(get bounds (:id child)) child]))) + (keep (fn [c] + (when-let [b (get bounds (:id c))] + [@b c])))) layout-data (gd/calc-layout-data child (:points child) children bounds objects true) auto-bounds (gb/layout-content-bounds bounds child layout-data)] - (max (ctl/child-min-height child) - (gpo/height-points auto-bounds))) + (if auto-bounds + (max (ctl/child-min-height child) + (gpo/height-points auto-bounds)) + (ctl/child-min-height child))) (ctl/fill-height? child) (ctl/child-min-height child) diff --git a/common/test/common_tests/geom_bounds_layout_nil_test.cljc b/common/test/common_tests/geom_bounds_layout_nil_test.cljc new file mode 100644 index 0000000000..db070ce4f4 --- /dev/null +++ b/common/test/common_tests/geom_bounds_layout_nil_test.cljc @@ -0,0 +1,213 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns common-tests.geom-bounds-layout-nil-test + (:require + [app.common.data :as d] + [app.common.geom.bounds-map :as gbm] + [app.common.geom.point :as gpt] + [app.common.geom.rect :as grc] + [app.common.geom.shapes.flex-layout.bounds :as fb] + [app.common.geom.shapes.grid-layout.bounds :as gb] + [app.common.geom.shapes.min-size-layout :as msl] + [app.common.types.shape :as cts] + [app.common.types.shape.layout :as ctl] + [app.common.uuid :as uuid] + [clojure.test :as t])) + +;; ---- Helpers ---- + +(defn- make-rect + [id x y w h] + (-> (cts/setup-shape {:id id + :type :rect + :name (str "rect-" id) + :x x :y y :width w :height h}) + (assoc :parent-id uuid/zero + :frame-id uuid/zero))) + +(defn- make-flex-frame + [id child-ids & {:keys [x y w h dir] + :or {x 0 y 0 w 200 h 200 dir :row}}] + (-> (cts/setup-shape {:id id + :type :frame + :name (str "flex-" id) + :layout :flex + :layout-flex-dir dir + :x x :y y :width w :height h}) + (assoc :parent-id uuid/zero + :frame-id uuid/zero + :shapes (vec child-ids)))) + +(defn- make-grid-frame + [id child-ids & {:keys [x y w h dir] + :or {x 0 y 0 w 200 h 200 dir :row}}] + (let [cell-id (uuid/next)] + (-> (cts/setup-shape {:id id + :type :frame + :name (str "grid-" id) + :layout :grid + :layout-grid-dir dir + :layout-grid-columns [{:type :flex :value 1}] + :layout-grid-rows [{:type :flex :value 1}] + :layout-grid-cells + {cell-id {:id cell-id + :row 1 + :row-span 1 + :column 1 + :column-span 1 + :shapes (vec child-ids)}} + :layout-padding-type :multiple + :layout-padding {:p1 0 :p2 0 :p3 0 :p4 0} + :layout-gap {:column-gap 0 :row-gap 0} + :x x :y y :width w :height h}) + (assoc :parent-id uuid/zero + :frame-id uuid/zero + :shapes (vec child-ids))))) + +(defn- make-objects + [shapes] + (let [shape-map (into {} (map (fn [s] [(:id s) s]) shapes))] + (reduce-kv (fn [m _id shape] + (if (contains? shape :shapes) + (reduce (fn [m' child-id] + (assoc-in m' [child-id :parent-id] (:id shape))) + m + (:shapes shape)) + m)) + shape-map + shape-map))) + +(defn- bounds-map-from-objects + "Build a bounds map from objects, optionally excluding some IDs." + [objects & {:keys [exclude-ids]}] + (let [full (gbm/objects->bounds-map objects)] + (if (seq exclude-ids) + (apply dissoc full exclude-ids) + full))) + +;; ---- Tests for flex layout bounds with nil bounds ---- + +(t/deftest layout-content-points-with-missing-parent-bounds + (t/testing "layout-content-points returns nil when parent is not in bounds map" + (let [child-id (uuid/next) + parent-id (uuid/next) + child (make-rect child-id 10 10 50 50) + parent (make-flex-frame parent-id [child-id]) + objects (make-objects [parent child]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id})] + + (t/is (nil? (fb/layout-content-points bounds parent [child] objects)))))) + +(t/deftest layout-content-points-with-missing-child-bounds + (t/testing "layout-content-points skips children with missing bounds" + (let [child1-id (uuid/next) + child2-id (uuid/next) + parent-id (uuid/next) + child1 (make-rect child1-id 10 10 50 50) + child2 (make-rect child2-id 70 10 50 50) + parent (make-flex-frame parent-id [child1-id child2-id]) + objects (make-objects [parent child1 child2]) + bounds (bounds-map-from-objects objects :exclude-ids #{child1-id})] + + (let [result (fb/layout-content-points bounds parent [child1 child2] objects)] + (t/is (some? result)) + ;; Only child2's bounds should be in the result + (t/is (pos? (count result))))))) + +(t/deftest layout-content-bounds-with-missing-parent-bounds + (t/testing "layout-content-bounds returns nil when parent is not in bounds map" + (let [child-id (uuid/next) + parent-id (uuid/next) + child (make-rect child-id 10 10 50 50) + parent (make-flex-frame parent-id [child-id]) + objects (make-objects [parent child]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id})] + + (t/is (nil? (fb/layout-content-bounds bounds parent [child] objects)))))) + +;; ---- Tests for grid layout bounds with nil bounds ---- + +(t/deftest grid-layout-content-points-with-missing-parent-bounds + (t/testing "grid layout-content-points returns nil when parent is not in bounds map" + (let [parent-id (uuid/next) + parent (make-grid-frame parent-id []) + objects (make-objects [parent]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id}) + layout-data {:row-tracks [{:start-p (gpt/point 0 0) :size 100}] + :column-tracks [{:start-p (gpt/point 0 0) :size 100}]}] + + (t/is (nil? (gb/layout-content-points bounds parent layout-data)))))) + +(t/deftest grid-layout-content-bounds-with-missing-parent-bounds + (t/testing "grid layout-content-bounds returns nil when parent is not in bounds map" + (let [parent-id (uuid/next) + parent (make-grid-frame parent-id []) + objects (make-objects [parent]) + bounds (bounds-map-from-objects objects :exclude-ids #{parent-id}) + layout-data {:row-tracks [{:start-p (gpt/point 0 0) :size 100}] + :column-tracks [{:start-p (gpt/point 0 0) :size 100}]}] + + (t/is (nil? (gb/layout-content-bounds bounds parent layout-data)))))) + +;; ---- Tests for min-size-layout with nil bounds ---- + +(t/deftest child-min-width-grid-with-missing-child-bounds + (t/testing "child-min-width falls back when grid layout child bounds are missing" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-grid-dir :row + :layout-item-h-sizing :fill)) + objects (make-objects [child grandchild]) + ;; Exclude grandchild from bounds to simulate missing entry + bounds (bounds-map-from-objects objects :exclude-ids #{grandchild-id}) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (let [result (msl/child-min-width child child-bounds bounds objects)] + (t/is (= (ctl/child-min-width child) result)))))) + +(t/deftest child-min-height-grid-with-missing-child-bounds + (t/testing "child-min-height falls back when grid layout child bounds are missing" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-grid-dir :column + :layout-item-v-sizing :fill)) + objects (make-objects [child grandchild]) + bounds (bounds-map-from-objects objects :exclude-ids #{grandchild-id}) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (let [result (msl/child-min-height child child-bounds bounds objects)] + (t/is (= (ctl/child-min-height child) result)))))) + +(t/deftest child-min-width-grid-with-present-child-bounds + (t/testing "child-min-width handles bounded children in a fill-width grid" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-item-h-sizing :fill)) + objects (make-objects [child grandchild]) + bounds (bounds-map-from-objects objects) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (t/is (number? (msl/child-min-width child child-bounds bounds objects)))))) + +(t/deftest child-min-height-grid-with-present-child-bounds + (t/testing "child-min-height handles bounded children in a fill-height grid" + (let [grandchild-id (uuid/next) + child-id (uuid/next) + grandchild (make-rect grandchild-id 0 0 30 30) + child (-> (make-grid-frame child-id [grandchild-id] :w 100 :h 100) + (assoc :layout-item-v-sizing :fill)) + objects (make-objects [child grandchild]) + bounds (bounds-map-from-objects objects) + child-bounds (grc/rect->points (grc/make-rect 0 0 100 100))] + + (t/is (number? (msl/child-min-height child child-bounds bounds objects)))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index df45090131..84930589d2 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -23,6 +23,7 @@ [common-tests.files.shapes-builder-test] [common-tests.files.validate-test] [common-tests.geom-align-test] + [common-tests.geom-bounds-layout-nil-test] [common-tests.geom-bounds-map-test] [common-tests.geom-flex-layout-test] [common-tests.geom-grid-layout-test] @@ -99,6 +100,7 @@ 'common-tests.files-migrations-test 'common-tests.files.validate-test 'common-tests.geom-align-test + 'common-tests.geom-bounds-layout-nil-test 'common-tests.geom-bounds-map-test 'common-tests.geom-flex-layout-test 'common-tests.geom-grid-layout-test diff --git a/frontend/src/app/main/data/exports/assets.cljs b/frontend/src/app/main/data/exports/assets.cljs index 83a7cbb5fa..b1ced71ad6 100644 --- a/frontend/src/app/main/data/exports/assets.cljs +++ b/frontend/src/app/main/data/exports/assets.cljs @@ -20,10 +20,27 @@ [app.util.dom :as dom] [app.util.websocket :as ws] [beicon.v2.core :as rx] + [cuerdas.core :as str] [potok.v2.core :as ptk])) (def default-timeout 5000) +(defn normalize-export + [{:keys [object-id name] :as export}] + (assoc export :name (if (str/blank? name) + (str object-id) + name))) + +(defn- normalize-exports + [exports] + (mapv normalize-export exports)) + +(defn- normalize-export-shapes-params + [{:keys [exports] :as params}] + (cond-> params + (seq exports) + (assoc :exports (normalize-exports exports)))) + (defn toggle-detail-visibililty [] (ptk/reify ::toggle-detail-visibililty @@ -190,100 +207,102 @@ (defn request-simple-export [{:keys [export]}] - (ptk/reify ::request-simple-export - ptk/UpdateEvent - (update [_ state] - (cond-> state - (not (use-wasm-export? state export)) - (update :export assoc :in-progress true :id uuid/zero))) + (let [export (normalize-export export)] + (ptk/reify ::request-simple-export + ptk/UpdateEvent + (update [_ state] + (cond-> state + (not (use-wasm-export? state export)) + (update :export assoc :in-progress true :id uuid/zero))) - ptk/WatchEvent - (watch [_ state _] - (if (use-wasm-export? state export) - (rx/of (request-simple-export-wasm export)) - (let [profile-id (:profile-id state) - params {:exports [export] - :profile-id profile-id - :cmd :export-shapes - :wait true - :is-wasm (wasm-export-enabled? state)}] - (rx/concat - (dwp/force-persist-and-wait 400) + ptk/WatchEvent + (watch [_ state _] + (if (use-wasm-export? state export) + (rx/of (request-simple-export-wasm export)) + (let [profile-id (:profile-id state) + params (normalize-export-shapes-params {:exports [export] + :profile-id profile-id + :cmd :export-shapes + :wait true + :is-wasm (wasm-export-enabled? state)})] + (rx/concat + (dwp/force-persist-and-wait 400) - (->> (rp/cmd! :export params) - (rx/map (fn [{:keys [filename mtype uri]}] - (dom/trigger-download-uri filename mtype uri) - (clear-export-state uuid/zero))) - (rx/catch (fn [cause] - (rx/concat - (rx/of (clear-export-state uuid/zero)) - (rx/throw cause))))))))))) + (->> (rp/cmd! :export params) + (rx/map (fn [{:keys [filename mtype uri]}] + (dom/trigger-download-uri filename mtype uri) + (clear-export-state uuid/zero))) + (rx/catch (fn [cause] + (rx/concat + (rx/of (clear-export-state uuid/zero)) + (rx/throw cause)))))))))))) (defn request-multiple-export [{:keys [exports cmd name] :or {cmd :export-shapes} :as params}] - (ptk/reify ::request-multiple-export - ptk/WatchEvent - (watch [_ state _] - (let [resource-id (volatile! nil) - profile-id (:profile-id state) - ws-conn (:ws-conn state) - params (cond-> - {:exports exports - :cmd cmd - :profile-id profile-id - :force-multiple true - :is-wasm (wasm-export-enabled? state)} - (some? name) - (assoc :name name)) + (let [exports (normalize-exports exports)] + (ptk/reify ::request-multiple-export + ptk/WatchEvent + (watch [_ state _] + (let [resource-id (volatile! nil) + profile-id (:profile-id state) + ws-conn (:ws-conn state) + params (cond-> + {:exports exports + :cmd cmd + :profile-id profile-id + :force-multiple true + :is-wasm (wasm-export-enabled? state)} + (some? name) + (assoc :name name)) - progress-stream - (->> (ws/get-rcv-stream ws-conn) - (rx/filter ws/message-event?) - (rx/map :payload) - (rx/filter #(= :export-update (:type %))) - (rx/filter #(= @resource-id (:resource-id %))) - (rx/share)) + progress-stream + (->> (ws/get-rcv-stream ws-conn) + (rx/filter ws/message-event?) + (rx/map :payload) + (rx/filter #(= :export-update (:type %))) + (rx/filter #(= @resource-id (:resource-id %))) + (rx/share)) - stopper - (rx/filter #(or (= "ended" (:status %)) - (= "error" (:status %))) - progress-stream)] + stopper + (rx/filter #(or (= "ended" (:status %)) + (= "error" (:status %))) + progress-stream)] - (swap! st/ongoing-tasks conj :export) + (swap! st/ongoing-tasks conj :export) - (rx/merge - ;; Force that all data is persisted; best effort. - (rx/of ::dwp/force-persist) + (rx/merge + ;; Force that all data is persisted; best effort. + (rx/of ::dwp/force-persist) - ;; Launch the exportation process and stores the resource id - ;; locally. - (->> (rp/cmd! :export params) - (rx/map (fn [{:keys [id] :as resource}] - (vreset! resource-id id) - (initialize-export-status exports cmd resource)))) + ;; Launch the exportation process and stores the resource id + ;; locally. + (->> (rp/cmd! :export params) + (rx/map (fn [{:keys [id] :as resource}] + (vreset! resource-id id) + (initialize-export-status exports cmd resource)))) - ;; We proceed to update the export state with incoming - ;; progress updates. We delay the stopper for give some time - ;; to update the status with ended or errored status before - ;; close the stream. - (->> progress-stream - (rx/map update-export-status) - (rx/take-until (rx/delay 500 stopper)) - (rx/finalize (fn [] - (swap! st/ongoing-tasks disj :export)))) + ;; We proceed to update the export state with incoming + ;; progress updates. We delay the stopper for give some time + ;; to update the status with ended or errored status before + ;; close the stream. + (->> progress-stream + (rx/map update-export-status) + (rx/take-until (rx/delay 500 stopper)) + (rx/finalize (fn [] + (swap! st/ongoing-tasks disj :export)))) - ;; We hide need to hide the ui elements of the export after - ;; some interval. We also delay a little bit more the stopper - ;; for ensure that after some security time, the stream is - ;; completely closed. - (->> progress-stream - (rx/filter #(= "ended" (:status %))) - (rx/take 1) - (rx/delay default-timeout) - (rx/map #(clear-export-state @resource-id)) - (rx/take-until (rx/delay 6000 stopper)))))))) + ;; We hide need to hide the ui elements of the export after + ;; some interval. We also delay a little bit more the stopper + ;; for ensure that after some security time, the stream is + ;; completely closed. + (->> progress-stream + (rx/filter #(= "ended" (:status %))) + (rx/take 1) + (rx/delay default-timeout) + (rx/map #(clear-export-state @resource-id)) + (rx/take-until (rx/delay 6000 stopper))))))))) (defn request-export [{:keys [exports] :as params}] diff --git a/frontend/src/app/main/data/workspace/clipboard.cljs b/frontend/src/app/main/data/workspace/clipboard.cljs index 3e50a4c1e3..ac1fb64e9a 100644 --- a/frontend/src/app/main/data/workspace/clipboard.cljs +++ b/frontend/src/app/main/data/workspace/clipboard.cljs @@ -34,6 +34,7 @@ [app.config :as cf] [app.main.data.changes :as dch] [app.main.data.event :as ev] + [app.main.data.exports.assets :as de] [app.main.data.exports.wasm :as wasm.exports] [app.main.data.helpers :as dsh] [app.main.data.notifications :as ntf] @@ -1174,16 +1175,16 @@ page-id (:current-page-id state) selected (first (dsh/lookup-selected state)) - export {:file-id file-id - :page-id page-id - :object-id selected - ;; webp would be preferrable, but PNG is the most supported image MIME type by clipboard APIs. - :type :png - ;; Always use 2 to ensure good enough quality for wireframes. - :scale 2 - :suffix "" - :enabled true - :name ""} + export (de/normalize-export {:file-id file-id + :page-id page-id + :object-id selected + ;; webp would be preferrable, but PNG is the most supported image MIME type by clipboard APIs. + :type :png + ;; Always use 2 to ensure good enough quality for wireframes. + :scale 2 + :suffix "" + :enabled true + :name ""}) ;; Create a deferred promise immediately, before any async operations. ;; Registering the clipboard write NOW preserves the user-gesture security diff --git a/frontend/src/app/main/data/workspace/colors.cljs b/frontend/src/app/main/data/workspace/colors.cljs index f8c916f881..a844bb5a40 100644 --- a/frontend/src/app/main/data/workspace/colors.cljs +++ b/frontend/src/app/main/data/workspace/colors.cljs @@ -9,6 +9,7 @@ [app.common.data :as d] [app.common.data.macros :as dm] [app.common.files.helpers :as cfh] + [app.common.math :as mth] [app.common.schema :as sm] [app.common.types.color :as clr] [app.common.types.fills :as types.fills] @@ -955,7 +956,8 @@ (or (not cap-stops?) (< (count stops) types.fills/MAX-GRADIENT-STOPS))] (if can-add-stop? - (let [new-stop (-> (clr/interpolate-gradient stops offset) + (let [offset (mth/clamp offset 0 1) + new-stop (-> (clr/interpolate-gradient stops offset) (split-color-components)) stops (conj stops new-stop) stops (into [] (sort-by :offset stops)) @@ -978,7 +980,8 @@ stops (mapv split-color-components (if cap-stops? (take types.fills/MAX-GRADIENT-STOPS stops) - stops))] + stops)) + stops (mapv #(update % :offset (fn [o] (mth/clamp o 0 1))) stops)] (-> state (assoc :current-color (get stops stop)) (assoc :stops stops)))))))) diff --git a/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs b/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs index ff8b733433..7d1c7f14cc 100644 --- a/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs +++ b/frontend/src/app/main/ui/workspace/colorpicker/gradients.cljs @@ -233,6 +233,7 @@ (mf/deps on-add-stop-preview) (fn [^js e] (let [offset (-> (event->offset e) + (mth/clamp 0 1) (mth/precision 2))] (when on-add-stop-preview (on-add-stop-preview offset))))) diff --git a/frontend/src/app/main/ui/workspace/viewport/gradients.cljs b/frontend/src/app/main/ui/workspace/viewport/gradients.cljs index 7f1abe92e9..ec2f9b586c 100644 --- a/frontend/src/app/main/ui/workspace/viewport/gradients.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/gradients.cljs @@ -189,6 +189,7 @@ lv (-> (gpt/to-vec from-p to-p) (gpt/unit)) nv (gpt/normal-left lv) offset (-> (gsp/project-t position [from-p to-p] nv) + (mth/clamp 0 1) (mth/precision 2)) new-stop (cc/interpolate-gradient stops offset) stops (conj stops new-stop) diff --git a/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs b/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs index afc461b4c6..c756f06e52 100644 --- a/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs @@ -56,51 +56,52 @@ (defn process-pointer-move [viewport-node canvas canvas-image-data zoom-view-context last-picked-color client-x client-y] - (when-let [image-data (mf/ref-val canvas-image-data)] - (when-let [zoom-view-node (dom/get-element "picker-detail")] - (when-not (mf/ref-val zoom-view-context) - (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) - (let [canvas-width 260 - canvas-height 140 - {brx :left bry :top} (dom/get-bounding-rect viewport-node) + (when viewport-node + (when-let [image-data (mf/ref-val canvas-image-data)] + (when-let [zoom-view-node (dom/get-element "picker-detail")] + (when-not (mf/ref-val zoom-view-context) + (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) + (let [canvas-width 260 + canvas-height 140 + {brx :left bry :top} (dom/get-bounding-rect viewport-node) - x (mth/floor (- client-x brx)) - y (mth/floor (- client-y bry)) + x (mth/floor (- client-x brx)) + y (mth/floor (- client-y bry)) - img-width (unchecked-get image-data "width") - img-height (unchecked-get image-data "height") + img-width (unchecked-get image-data "width") + img-height (unchecked-get image-data "height") - zoom-context (mf/ref-val zoom-view-context) + zoom-context (mf/ref-val zoom-view-context) - sx (- x 32) - sy (if (cfg/check-browser? :safari) y (- y 17)) - sw 65 - sh 35 - dx 0 - dy 0 - dw canvas-width - dh canvas-height] + sx (- x 32) + sy (if (cfg/check-browser? :safari) y (- y 17)) + sw 65 + sh 35 + dx 0 + dy 0 + dw canvas-width + dh canvas-height] - (when (obj/get zoom-context "imageSmoothingEnabled") - (obj/set! zoom-context "imageSmoothingEnabled" false)) - (.clearRect zoom-context 0 0 canvas-width canvas-height) - (.drawImage zoom-context canvas sx sy sw sh dx dy dw dh) + (when (obj/get zoom-context "imageSmoothingEnabled") + (obj/set! zoom-context "imageSmoothingEnabled" false)) + (.clearRect zoom-context 0 0 canvas-width canvas-height) + (.drawImage zoom-context canvas sx sy sw sh dx dy dw dh) - ;; Only pick color when cursor is within canvas bounds to avoid garbage pixels - (when (and (>= x 0) (< x img-width) (>= y 0) (< y img-height)) - (let [offset (* (+ (* y img-width) x) 4) - rgba (unchecked-get image-data "data") - r (d/check-num (obj/get rgba (+ 0 offset)) 255) - g (d/check-num (obj/get rgba (+ 1 offset)) 255) - b (d/check-num (obj/get rgba (+ 2 offset)) 255) - a (d/check-num (obj/get rgba (+ 3 offset)) 255) - color [r g b a]] - ;; Store latest color synchronously so the click handler always reads - ;; the correct pixel even before the rAF fires (fixes race condition) - (mf/set-ref-val! last-picked-color color) - (timers/raf - (fn [] - (st/emit! (dwc/pick-color color)))))))))) + ;; Only pick color when cursor is within canvas bounds to avoid garbage pixels + (when (and (>= x 0) (< x img-width) (>= y 0) (< y img-height)) + (let [offset (* (+ (* y img-width) x) 4) + rgba (unchecked-get image-data "data") + r (d/check-num (obj/get rgba (+ 0 offset)) 255) + g (d/check-num (obj/get rgba (+ 1 offset)) 255) + b (d/check-num (obj/get rgba (+ 2 offset)) 255) + a (d/check-num (obj/get rgba (+ 3 offset)) 255) + color [r g b a]] + ;; Store latest color synchronously so the click handler always reads + ;; the correct pixel even before the rAF fires (fixes race condition) + (mf/set-ref-val! last-picked-color color) + (timers/raf + (fn [] + (st/emit! (dwc/pick-color color))))))))))) (mf/defc pixel-overlay* @@ -260,18 +261,19 @@ (defn- viewport->canvas-coords "Maps client (viewport) coordinates to device-pixel canvas coordinates." [viewport-node client-x client-y] - (let [{brx :left bry :top} (dom/get-bounding-rect viewport-node) - dpr (wasm.api/get-dpr) - x (mth/floor (- client-x brx)) - y (mth/floor (- client-y bry))] - [(mth/floor (* x dpr)) - (mth/floor (* y dpr))])) + (when viewport-node + (let [{brx :left bry :top} (dom/get-bounding-rect viewport-node) + dpr (wasm.api/get-dpr) + x (mth/floor (- client-x brx)) + y (mth/floor (- client-y bry))] + [(mth/floor (* x dpr)) + (mth/floor (* y dpr))]))) (defn process-pointer-move-wasm "Updates the magnifier loupe with the canvas region under the cursor. The actual color is only read on click (see `pick-color-at-wasm`)." [viewport-node canvas zoom-view-context client-x client-y] - (when canvas + (when (and canvas viewport-node) (when-let [zoom-view-node (dom/get-element "picker-detail")] (when-not (mf/ref-val zoom-view-context) (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) @@ -309,7 +311,7 @@ the correct color even on GPUs where a raw WebGL `readPixels` returned values with their byte order swapped." [viewport-node canvas client-x client-y] - (when canvas + (when (and canvas viewport-node) (let [[canvas-x canvas-y] (viewport->canvas-coords viewport-node client-x client-y) img-width (.-width canvas) img-height (.-height canvas)] @@ -370,7 +372,7 @@ handle-draw-picker-canvas (mf/use-callback (fn [] - (when canvas + (when (and canvas viewport-node) ;; Read current mouse position from ref so the loupe refreshes on ;; each render even without a mouse-move. (let [{mx :x my :y} (mf/ref-val initial-mouse-pos)] diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 8375c561e5..88177b39ab 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -34,6 +34,7 @@ [app.common.types.text :as txt] [app.common.uuid :as uuid] [app.config :as cf] + [app.main.data.exports.assets :as de] [app.main.data.exports.wasm :as wasm.exports] [app.main.data.persistence :as dwp] [app.main.data.plugins :as dp] @@ -1560,13 +1561,13 @@ :profile-id (:profile-id @st/state) :wait true :is-wasm false - :exports [{:file-id file-id - :page-id page-id - :object-id id - :name (:name shape) - :type (:type value :png) - :suffix (:suffix value "") - :scale (:scale value 1)}]}] + :exports [(de/normalize-export {:file-id file-id + :page-id page-id + :object-id id + :name (:name shape) + :type (:type value :png) + :suffix (:suffix value "") + :scale (:scale value 1)})]}] (js/Promise. (fn [resolve reject] ;; The exporter renders the file from its persisted diff --git a/frontend/test/frontend_tests/data/exports_assets_test.cljs b/frontend/test/frontend_tests/data/exports_assets_test.cljs new file mode 100644 index 0000000000..d4ae9edea8 --- /dev/null +++ b/frontend/test/frontend_tests/data/exports_assets_test.cljs @@ -0,0 +1,99 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns frontend-tests.data.exports-assets-test + (:require + [app.common.uuid :as uuid] + [app.main.data.exports.assets :as de] + [app.main.data.persistence :as dwp] + [app.main.repo :as repo] + [app.main.store :as st] + [app.util.dom :as dom] + [app.util.websocket :as ws] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.events :as the] + [frontend-tests.helpers.mock :as mock] + [potok.v2.core :as ptk])) + +(def ^:private export {:id (uuid/next) + :object-id (uuid/next) + :type :png + :suffix "" + :scale 1}) + +(defn- export-with-name + [name] + (merge export {:name name})) + +(defn- test-state + [] + {:profile-id (:id export) + :ws-conn nil}) + +(t/deftest normalize-export-preserves-existing-name + (t/is (= (export-with-name "Layer 1") + (de/normalize-export (export-with-name "Layer 1"))))) + +(t/deftest normalize-export-replaces-nil-name-with-object-id + (t/is (= (export-with-name (str (:object-id export))) + (de/normalize-export (assoc export :name nil))))) + +(t/deftest normalize-export-replaces-empty-name-with-object-id + (t/is (= (export-with-name (str (:object-id export))) + (de/normalize-export (assoc export :name ""))))) + +(t/deftest request-simple-export-sends-normalized-export + (t/async done + (let [export (export-with-name "") + observed (atom nil)] + (mock/with-mocks {repo/cmd! (mock/stub (fn [_ params] + (reset! observed params) + (rx/of {:filename "export.png" + :mtype "image/png" + :uri "blob:export"}))) + dwp/force-persist-and-wait (mock/stub (fn [_] (rx/of ::force-persisted))) + dom/trigger-download-uri (mock/stub (fn [& _] nil))} + (fn [done'] + (let [completed (fn [_state] + (t/is (= (export-with-name (str (:object-id export))) + (-> @observed :exports first))))] + (ptk/emit! (the/prepare-store (test-state) done' completed) + (de/request-simple-export {:export export}) + :the/end))) + done)))) + +(t/deftest request-multiple-export-sends-normalized-enabled-exports + (t/async done + (let [exports [{:id "enabled-1" + :object-id "enabled-1" + :shape {:id "enabled-1"} + :type :png + :suffix "" + :scale 1 + :enabled true + :name ""}] + observed (atom nil)] + (mock/with-mocks {repo/cmd! (mock/stub (fn [_ params] + (reset! observed params) + (rx/of {:id (:id export)}))) + ws/get-rcv-stream (mock/stub (fn [_] (rx/empty))) + st/ongoing-tasks (atom #{})} + (fn [done'] + (let [completed (fn [_state] + (t/is (= [{:id "enabled-1" + :object-id "enabled-1" + :shape {:id "enabled-1"} + :type :png + :suffix "" + :scale 1 + :enabled true + :name "enabled-1"}] + (:exports @observed))))] + (ptk/emit! (the/prepare-store (test-state) done' completed) + (de/request-multiple-export {:exports exports}) + :the/end))) + done)))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index f7c58d2aea..21c7d23c06 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -9,6 +9,7 @@ [frontend-tests.composable-tests.comp.sync-test] [frontend-tests.copy-as-svg-test] [frontend-tests.data.dashboard-test] + [frontend-tests.data.exports-assets-test] [frontend-tests.data.nitrate-test] [frontend-tests.data.repo-test] [frontend-tests.data.store-test] @@ -101,7 +102,8 @@ 'frontend-tests.data.dashboard-test 'frontend-tests.data.nitrate-test 'frontend-tests.data.repo-test - + 'frontend-tests.data.store-test + 'frontend-tests.data.exports-assets-test 'frontend-tests.data.uploads-test 'frontend-tests.data.viewer-test 'frontend-tests.data.workspace-colors-test diff --git a/mcp/README.md b/mcp/README.md index ac99679f28..9b4ac77038 100644 --- a/mcp/README.md +++ b/mcp/README.md @@ -267,6 +267,7 @@ The Penpot MCP server can be configured using environment variables. | `PENPOT_MCP_REPL_PORT` | Port for the REPL server (development/debugging) | `4403` | | `PENPOT_MCP_REMOTE_MODE` | Enable remote mode (disables file system access). Set to `true` to enable. | `false` | | `PENPOT_MCP_DEVENV` | Enable Penpot development environment tools. Set to `true` to enable. | `false` | +| `PENPOT_MCP_TOOL_TIMEOUT_S` | Timeout, in seconds, for tool calls dispatched to the Penpot plugin | `120` | | `PENPOT_MCP_EXPORT_SHAPE_MAX_PARALLEL_REQUESTS` | Maximum number of parallel export shape requests (multi-user mode only). | `0` (no limit) | | `PENPOT_MCP_REDIS_URI` | Redis connection URI (e.g. `redis://host:6379`) enabling multi-instance horizontal scaling via Redis pub/sub task routing (multi-user mode only). When unset, the server runs in single-instance mode, requiring the plugin and MCP client to connect to the same instance. | (unset) | diff --git a/mcp/packages/server/src/PenpotMcpServer.ts b/mcp/packages/server/src/PenpotMcpServer.ts index 8a4ee30f25..bd992ec108 100644 --- a/mcp/packages/server/src/PenpotMcpServer.ts +++ b/mcp/packages/server/src/PenpotMcpServer.ts @@ -127,6 +127,7 @@ export class PenpotMcpServer { this.webSocketPort = parseInt(process.env.PENPOT_MCP_WEBSOCKET_PORT ?? "4402", 10); this.replPort = parseInt(process.env.PENPOT_MCP_REPL_PORT ?? "4403", 10); this.tenant = process.env.PENPOT_TENANT ?? "default"; + const toolTimeoutSecs = parseInt(process.env.PENPOT_MCP_TOOL_TIMEOUT_S ?? "120", 10); this.configLoader = new ConfigurationLoader(process.cwd()); this.apiDocs = new ApiDocs(); @@ -147,7 +148,7 @@ export class PenpotMcpServer { this.redisBridge = new RedisBridge(redisUri, this.tenant); } - this.pluginBridge = new PluginBridge(this, this.webSocketPort, this.redisBridge); + this.pluginBridge = new PluginBridge(this, this.webSocketPort, toolTimeoutSecs, this.redisBridge); this.replServer = new ReplServer(this.pluginBridge, this.replPort, this.host); } diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index 19a3a77545..a362faef36 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -66,8 +66,11 @@ export function assertPluginResponsive( * over these connections. */ export class PluginBridge { + public static readonly MULTIUSER_CONNECTION_ERROR_MESSAGE = `No Penpot instance connected for user token. Please ensure that Penpot is connected and that the MCP client connection is using the correct token.`; + private readonly logger = createLogger("PluginBridge"); private readonly wsServer: WebSocketServer; + private readonly connectedClients: Map = new Map(); private readonly clientsByToken: Map = new Map(); private readonly pendingTasks: Map> = new Map(); @@ -83,12 +86,13 @@ export class PluginBridge { * holding the relevant plugin's WebSocket connection (which may be this same * instance) via Redis, rather than dispatched directly over a local socket. * @param taskTimeoutSecs - Timeout, in seconds, for plugin task execution + * (defaults to {@link DEFAULT_TASK_TIMEOUT_SECS}) */ constructor( public readonly mcpServer: PenpotMcpServer, private port: number, - private readonly redisBridge?: RedisBridge, - private taskTimeoutSecs: number = 30 + private readonly taskTimeoutSecs: number, + private readonly redisBridge?: RedisBridge ) { this.wsServer = new WebSocketServer({ port: port }); this.setupWebSocketHandlers(); @@ -195,9 +199,10 @@ export class PluginBridge { /** * Removes a client connection and releases all resources associated with it. * - * Clears the per-connection keep-alive interval and removes the connection - * from both the socket-keyed and token-keyed indexes. Safe to call with a - * socket that is not (or no longer) registered. + * Clears the per-connection keep-alive interval and removes the connection from the + * socket-keyed index. The token-keyed index entry (and, in multi-instance mode, the + * token's Redis task subscription) is removed only if it is owned by the given + * connection. Safe to call with a socket that is not (or no longer) registered. * * @param ws - The WebSocket whose connection state should be removed */ @@ -209,12 +214,18 @@ export class PluginBridge { clearInterval(connection.pingInterval); this.connectedClients.delete(ws); if (connection.userToken) { - this.clientsByToken.delete(connection.userToken); + // Perform the token-keyed cleanup only if this connection owns the token registration. + // A connection rejected as a duplicate carries the same token but must not remove token associations. + if (this.clientsByToken.get(connection.userToken) !== connection) { + this.logger.debug("Removed connection does not own its token registration; skipping token cleanup"); + } else { + this.clientsByToken.delete(connection.userToken); - if (this.redisBridge) { - this.redisBridge - .unsubscribeFromTasks(connection.userToken) - .catch((error) => this.logger.error(error, "Failed to unsubscribe from Redis task channel")); + if (this.redisBridge) { + this.redisBridge + .unsubscribeFromTasks(connection.userToken) + .catch((error) => this.logger.error(error, "Failed to unsubscribe from Redis task channel")); + } } } } @@ -253,6 +264,35 @@ export class PluginBridge { this.logger.info(`Task ${response.id} completed: success=${response.success}`); } + /** + * Rejects a still-pending task with the given error, releasing its correlation state. + * + * Clears the task's timeout (if armed) and removes the task from the pending-task + * index before rejecting its promise. Safe to call for a task that has already been + * settled (e.g. by a response or a timeout), in which case nothing happens. + * + * @param taskId - The ID of the task to reject + * @param error - The error with which to reject the task + * @returns Whether the task was still pending and has been rejected + */ + private rejectPendingTask(taskId: string, error: Error): boolean { + const pendingTask = this.pendingTasks.get(taskId); + if (!pendingTask) { + return false; + } + + const timeoutHandle = this.taskTimeouts.get(taskId); + if (timeoutHandle) { + clearTimeout(timeoutHandle); + this.taskTimeouts.delete(taskId); + } + this.pendingTasks.delete(taskId); + + pendingTask.rejectWithError(error); + this.logger.info(`Task ${taskId} rejected: ${error.message}`); + return true; + } + /** * Determines the client connection to use for executing a task. * @@ -271,9 +311,7 @@ export class PluginBridge { const connection = this.clientsByToken.get(sessionContext.userToken); if (!connection) { - throw new Error( - `No plugin instance connected for user token. Please ensure the plugin is running and connected with the correct token.` - ); + throw new Error(PluginBridge.MULTIUSER_CONNECTION_ERROR_MESSAGE); } return connection; @@ -321,6 +359,10 @@ export class PluginBridge { * `resolveWithResult`/`rejectWithError` methods. The same correlation and timeout * handling therefore applies regardless of the transport. * + * When routing via Redis, the task is rejected immediately (rather than timing out) + * if the published request reached no instance, i.e. if no instance holds a plugin + * connection for the session's user token, or if publishing fails outright. + * * @param task - The task to dispatch * @param useRedis - Whether to route the request via Redis (multi-instance) rather * than directly over the local WebSocket connection @@ -343,9 +385,17 @@ export class PluginBridge { // register the task for result correlation, then publish the request via Redis this.pendingTasks.set(task.id, task); - void redisBridge.sendTaskRequest(userToken, task.toRequest(), (response) => - this.handlePluginTaskResponse(response) - ); + void redisBridge + .sendTaskRequest(userToken, task.toRequest(), (response) => this.handlePluginTaskResponse(response)) + .then((receiverCount) => { + // fail fast when no instance received the request (no connection with matching user token in any instance) + if (receiverCount === 0) { + this.rejectPendingTask(task.id, new Error(PluginBridge.MULTIUSER_CONNECTION_ERROR_MESSAGE)); + } + }) + .catch((error) => { + this.rejectPendingTask(task.id, error instanceof Error ? error : new Error(String(error))); + }); // on timeout, release the response-channel subscription, since no response // will arrive to trigger its self-unsubscribe. @@ -367,14 +417,13 @@ export class PluginBridge { // Set up a timeout to reject the task if no response is received const timeoutHandle = setTimeout(() => { - const pendingTask = this.pendingTasks.get(task.id); - if (pendingTask) { - this.pendingTasks.delete(task.id); - this.taskTimeouts.delete(task.id); - onTimeout?.(); - pendingTask.rejectWithError( + if ( + this.rejectPendingTask( + task.id, new Error(`Task ${task.id} timed out after ${this.taskTimeoutSecs} seconds`) - ); + ) + ) { + onTimeout?.(); } }, this.taskTimeoutSecs * 1000); diff --git a/mcp/packages/server/src/RedisBridge.ts b/mcp/packages/server/src/RedisBridge.ts index d8117d9242..89c01a4871 100644 --- a/mcp/packages/server/src/RedisBridge.ts +++ b/mcp/packages/server/src/RedisBridge.ts @@ -91,12 +91,16 @@ export class RedisBridge { * @param userToken - The user token identifying the target plugin's request channel * @param request - The serialized plugin task request, passed through verbatim * @param onResponse - Handler invoked with the response when it arrives + * @returns The number of instances that received the request. A count of 0 means no + * instance is subscribed to the token's request channel (i.e. the plugin is not + * connected anywhere); the request was dropped, no response will ever arrive, and + * the response subscription has already been released. */ async sendTaskRequest( userToken: string, request: PluginTaskRequest, onResponse: TaskResponseHandler - ): Promise { + ): Promise { const responseChannel = this.responseChannel(request.id); const requestChannel = this.requestChannel(userToken); @@ -113,7 +117,19 @@ export class RedisBridge { await this.subscriber.subscribe(responseChannel); // publish only once the response subscription is confirmed - await this.publisher.publish(requestChannel, JSON.stringify(request)); + let receiverCount: number; + try { + receiverCount = await this.publisher.publish(requestChannel, JSON.stringify(request)); + } catch (error) { + // the request was never delivered, so no response can arrive + await this.unsubscribeFromResponse(request.id); + throw error; + } + if (receiverCount === 0) { + // no subscriber received the request, so no response can arrive + await this.unsubscribeFromResponse(request.id); + } + return receiverCount; } /**