From 9cc83c5c2bc3ce69a534e611ccb96da22ac48a9b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 11:43:46 +0000 Subject: [PATCH] :bug: Fix path editor code review findings Fix issues identified during code review of path editor enhancements: - Fix unused binding lint warning in distribute-content that blocked CI - Fix collision-step floor comparison to use round instead of floor, correctly detecting collisions when coordinates drift slightly below integer boundaries - Fix resolve-edit-fills to recurse through empty parent groups when searching for inherited fills in nested group hierarchies - Fix expand-coincident-node-indices to use fuzzy comparison (gpt/close?) instead of exact equality, handling floating-point divergence after transforms or rotations - Remove unreachable dead code in path-point* on-pointer-down handler - Add tests for collision-step boundary cases, nested group fill inheritance, and coincident node alignment/flipping AI-assisted-by: mimo-v2.5-pro --- .../src/app/common/types/path/selection.cljc | 4 +-- .../common_tests/types/path_data_test.cljc | 29 +++++++++++++++++++ .../main/data/workspace/path/clipboard.cljs | 2 +- .../app/main/data/workspace/path/edition.cljs | 6 +++- .../main/ui/workspace/shapes/path/editor.cljs | 20 +++++-------- .../data/workspace_path_edition_test.cljs | 10 +++++++ .../logic/path_clipboard_test.cljs | 18 ++++++++++++ 7 files changed, 73 insertions(+), 16 deletions(-) diff --git a/common/src/app/common/types/path/selection.cljc b/common/src/app/common/types/path/selection.cljc index 0e8cc29d98..826047431b 100644 --- a/common/src/app/common/types/path/selection.cljc +++ b/common/src/app/common/types/path/selection.cljc @@ -40,7 +40,7 @@ (comp (filter (fn [[_ segment]] (and (not= :close-path (:command segment)) - (contains? points (helpers/segment->point segment))))) + (some #(gpt/close? % (helpers/segment->point segment)) points)))) (map first)) (d/enumerate content)))) @@ -198,7 +198,7 @@ deltas (into {} (comp (map-indexed - (fn [k {:keys [point indices]}] + (fn [k {:keys [indices]}] (let [target (+ lo (* k step))] (map (fn [i] (let [node-point (get index->point i) diff --git a/common/test/common_tests/types/path_data_test.cljc b/common/test/common_tests/types/path_data_test.cljc index bac78c9e00..b094eb1f14 100644 --- a/common/test/common_tests/types/path_data_test.cljc +++ b/common/test/common_tests/types/path_data_test.cljc @@ -1514,6 +1514,35 @@ (t/is (= (gpt/point 10.0 0.0) (path.helpers/segment->point (nth result 1)))))) +(t/deftest segment-align-content-coincident-nodes + (t/testing "align-content groups coincident nodes with sub-epsilon coordinate differences" + (let [content (path/content + [{:command :move-to :params {:x 0.0 :y 0.0}} + {:command :line-to :params {:x 10.0001 :y 2.0}} + {:command :line-to :params {:x 10.0 :y 20.0}}]) + pts (fn [c] (mapv (comp (juxt :x :y) :params) (vec c)))] + ;; Nodes at ~10.0 should be grouped together for alignment + (t/is (= [[0.0 0.0] [0.0 2.0] [0.0 20.0]] + (pts (path/align-content content #{0 1 2} :hleft))))))) + +(t/deftest segment-flip-content-coincident-nodes + (t/testing "flip-content handles coincident nodes with sub-epsilon coordinate differences" + (let [content (path/content + [{:command :move-to :params {:x 0.0 :y 0.0}} + {:command :line-to :params {:x 10.0001 :y 0.0}} + {:command :line-to :params {:x 10.0 :y 10.0}}]) + pts (fn [c] (mapv (comp (juxt :x :y) :params) (vec c))) + result (path/flip-content content #{0 1 2} :horizontal)] + ;; All nodes should flip across the horizontal center. + ;; Floating-point imprecision from the 10.0001 input is expected. + (let [[[x0 y0] [x1 y1] [x2 y2]] (pts result)] + (t/is (mth/close? 10.0 x0 0.001)) + (t/is (mth/close? 0.0 y0 0.001)) + (t/is (mth/close? 0.0 x1 0.001)) + (t/is (mth/close? 0.0 y1 0.001)) + (t/is (mth/close? 0.0 x2 0.001)) + (t/is (mth/close? 10.0 y2 0.001)))))) + (t/deftest segment-set-handler-points (let [content (path/content [{:command :move-to :params {:x 0.0 :y 0.0}} diff --git a/frontend/src/app/main/data/workspace/path/clipboard.cljs b/frontend/src/app/main/data/workspace/path/clipboard.cljs index 504284e1bb..003b555e6b 100644 --- a/frontend/src/app/main/data/workspace/path/clipboard.cljs +++ b/frontend/src/app/main/data/workspace/path/clipboard.cljs @@ -68,7 +68,7 @@ y-step (/ (:y delta) (:y paste-offset))] (when (and (not (neg? x-step)) (mth/close? x-step y-step) - (mth/close? x-step (mth/floor x-step))) + (mth/close? x-step (mth/round x-step))) (long (mth/round x-step))))) (defn available-offset-step diff --git a/frontend/src/app/main/data/workspace/path/edition.cljs b/frontend/src/app/main/data/workspace/path/edition.cljs index fda97edd3a..fa6fb10646 100644 --- a/frontend/src/app/main/data/workspace/path/edition.cljs +++ b/frontend/src/app/main/data/workspace/path/edition.cljs @@ -883,7 +883,11 @@ (let [parent (get objects parent-id)] (cond (nil? parent) [] - (cfh/group-shape? parent) (svg-fills/resolve-shape-fills parent) + (cfh/group-shape? parent) (let [fills (svg-fills/resolve-shape-fills parent)] + (if (seq fills) + fills + (recur (:parent-id parent) + (conj visited parent-id)))) (cfh/frame-shape? parent) [] :else (recur (:parent-id parent) (conj visited parent-id))))))))) diff --git a/frontend/src/app/main/ui/workspace/shapes/path/editor.cljs b/frontend/src/app/main/ui/workspace/shapes/path/editor.cljs index 89d77e4e35..7c647087f1 100644 --- a/frontend/src/app/main/ui/workspace/shapes/path/editor.cljs +++ b/frontend/src/app/main/ui/workspace/shapes/path/editor.cljs @@ -119,19 +119,15 @@ (uwvv/capture-pointer event) (dom/stop-propagation event) (dom/prevent-default event) - ;; Preview nodes store their split params as metadata. - ;; FIXME: revisit this, using meta here breaks equality checks - (if (and is-new (some? (meta position))) - (st/emit! (drp/create-node-at-position (meta position))) - (let [is-shift (kbd/shift? event) - is-alt (kbd/alt? event) - is-mod (kbd/mod? event)] - (cond - is-move - (st/emit! (drp/start-move-path-point index is-shift is-alt is-mod)) + (let [is-shift (kbd/shift? event) + is-alt (kbd/alt? event) + is-mod (kbd/mod? event)] + (cond + is-move + (st/emit! (drp/start-move-path-point index is-shift is-alt is-mod)) - is-draw - (st/emit! (drp/on-draw-node-pointer-down index position is-alt is-mod)))))))] + is-draw + (st/emit! (drp/on-draw-node-pointer-down index position is-alt is-mod))))))] [:g.path-point [:circle.path-point diff --git a/frontend/test/frontend_tests/data/workspace_path_edition_test.cljs b/frontend/test/frontend_tests/data/workspace_path_edition_test.cljs index 2df0b96592..79b53d6a58 100644 --- a/frontend/test/frontend_tests/data/workspace_path_edition_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_path_edition_test.cljs @@ -95,3 +95,13 @@ result (path.edition/resolve-edit-fills shape objects)] ;; Should return empty fills instead of infinite loop (t/is (= [] result))))) + +(t/deftest resolve-edit-fills-with-empty-intermediate-group + (t/testing "resolve-edit-fills traverses past empty parent groups to find fills from ancestor" + (let [objects {1 {:type :group :parent-id 2 :fills []} + 2 {:type :group :parent-id 3 :fills [{:fill-color "#00ff00"}]} + 3 {:type :frame :parent-id nil :fills []}} + shape {:type :path :parent-id 1 :fills []} + result (path.edition/resolve-edit-fills shape objects)] + ;; Should inherit fills from OuterGroup through empty InnerGroup + (t/is (= [{:fill-color "#00ff00"}] result))))) diff --git a/frontend/test/frontend_tests/logic/path_clipboard_test.cljs b/frontend/test/frontend_tests/logic/path_clipboard_test.cljs index eba6281b39..5e79a7447a 100644 --- a/frontend/test/frontend_tests/logic/path_clipboard_test.cljs +++ b/frontend/test/frontend_tests/logic/path_clipboard_test.cljs @@ -237,3 +237,21 @@ step (path.clipboard/available-offset-step existing pasted)] ;; Should detect collision at step 1 and return step 0 as available (t/is (= 0 step))))) + +(t/deftest collision-step-with-coordinates-slightly-below-integer + (t/testing "collision-step detects collision when coordinates drift slightly below integer boundary" + (let [pasted (gpt/point 10.0 10.0) + existing (gpt/point 19.999 19.999) + step (path.clipboard/collision-step pasted existing)] + ;; x-step = 0.9999, y-step = 0.9999 + ;; With round-based check, these should be detected as collision at step 1 + (t/is (some? step)) + (t/is (= 1 step))))) + +(t/deftest available-offset-step-with-coordinates-slightly-below-integer + (t/testing "available-offset-step detects collision with sub-integer coordinate drift" + (let [existing #{(gpt/point 19.999 19.999)} + pasted #{(gpt/point 10.0 10.0)} + step (path.clipboard/available-offset-step existing pasted)] + ;; Should detect collision at step 1 and return step 0 as available + (t/is (= 0 step)))))