mirror of
https://github.com/penpot/penpot.git
synced 2026-03-27 13:50:26 +01:00
🐛 Fix nil deref on missing bounds in layout modifier propagation (#8735)
* 🐛 Fix nil deref on missing bounds in layout modifier propagation When a parent shape has a child ID in its shapes vector that does not exist in the objects map, the layout modifier code crashes because it derefs nil from the bounds map. The root cause is that children from the parent shapes list are not validated against the objects map before being passed to the layout modifier pipeline. Children with missing IDs pass through unchecked and reach apply-modifiers where bounds lookup fails. Fix by adding nil guards in apply-modifiers to skip children without bounds, and changing map to keep to filter them out. Signed-off-by: Andrey Antukh <niwi@niwi.nz> * 📎 Add tests for nil bounds in layout modifier propagation Tests cover flex and grid layout scenarios where a parent frame has child IDs in its shapes vector that do not exist in the objects map, verifying that set-objects-modifiers handles these gracefully without crashing. Tests: - Flex layout with normal children (baseline) - Flex layout with non-existent child in shapes - Flex layout with only non-existent children - Grid layout with non-existent child in shapes - Flex layout resize propagation with ghost children - Nested flex layout with non-existent child in outer frame Signed-off-by: Andrey Antukh <niwi@niwi.nz> --------- Signed-off-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Andrey Antukh
@@ -68,9 +68,10 @@
|
||||
[modif-tree children objects bounds parent transformed-parent-bounds]
|
||||
|
||||
(letfn [(apply-modifiers [bounds child]
|
||||
[(-> @(get bounds (:id child))
|
||||
(gpo/parent-coords-bounds @transformed-parent-bounds))
|
||||
child])
|
||||
(when-let [child-bounds (get bounds (:id child))]
|
||||
[(-> @child-bounds
|
||||
(gpo/parent-coords-bounds @transformed-parent-bounds))
|
||||
child]))
|
||||
|
||||
(set-child-modifiers [[layout-line modif-tree] [child-bounds child]]
|
||||
(let [[modifiers layout-line]
|
||||
@@ -83,7 +84,7 @@
|
||||
(->> children
|
||||
(keep (d/getf objects))
|
||||
(remove gco/invalid-geometry?)
|
||||
(map (partial apply-modifiers bounds)))
|
||||
(keep (partial apply-modifiers bounds)))
|
||||
|
||||
layout-data (gcfl/calc-layout-data parent @transformed-parent-bounds children bounds objects)
|
||||
children (into [] (cond-> children (not (:reverse? layout-data)) reverse))
|
||||
@@ -106,9 +107,10 @@
|
||||
[modif-tree objects bounds parent transformed-parent-bounds]
|
||||
|
||||
(letfn [(apply-modifiers [bounds child]
|
||||
[(-> @(get bounds (:id child))
|
||||
(gpo/parent-coords-bounds @transformed-parent-bounds))
|
||||
child])
|
||||
(when-let [child-bounds (get bounds (:id child))]
|
||||
[(-> @child-bounds
|
||||
(gpo/parent-coords-bounds @transformed-parent-bounds))
|
||||
child]))
|
||||
|
||||
(set-child-modifiers [modif-tree grid-data cell-data [child-bounds child]]
|
||||
(let [modifiers
|
||||
@@ -119,7 +121,7 @@
|
||||
|
||||
children
|
||||
(->> (cfh/get-immediate-children objects (:id parent) {:remove-hidden true})
|
||||
(map (partial apply-modifiers bounds)))
|
||||
(keep (partial apply-modifiers bounds)))
|
||||
grid-data (gcgl/calc-layout-data parent @transformed-parent-bounds children bounds objects)]
|
||||
(loop [modif-tree modif-tree
|
||||
bound+child (first children)
|
||||
@@ -240,8 +242,10 @@
|
||||
(gcfl/layout-content-bounds bounds parent children objects)
|
||||
|
||||
(ctl/grid-layout? parent)
|
||||
(let [children (->> children
|
||||
(map (fn [child] [@(get bounds (:id child)) child])))
|
||||
(let [children (->> children
|
||||
(keep (fn [child]
|
||||
(when-let [child-bounds-ref (get bounds (:id child))]
|
||||
[@child-bounds-ref child]))))
|
||||
layout-data (gcgl/calc-layout-data parent @parent-bounds children bounds objects)]
|
||||
(gcgl/layout-content-bounds bounds parent layout-data))))
|
||||
|
||||
|
||||
189
common/test/common_tests/geom_modifiers_test.cljc
Normal file
189
common/test/common_tests/geom_modifiers_test.cljc
Normal file
@@ -0,0 +1,189 @@
|
||||
;; 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 common-tests.geom-modifiers-test
|
||||
(:require
|
||||
[app.common.geom.modifiers :as gm]
|
||||
[app.common.geom.point :as gpt]
|
||||
[app.common.test-helpers.compositions :as tho]
|
||||
[app.common.test-helpers.files :as thf]
|
||||
[app.common.test-helpers.ids-map :as thi]
|
||||
[app.common.test-helpers.shapes :as ths]
|
||||
[app.common.types.modifiers :as ctm]
|
||||
[clojure.test :as t]))
|
||||
|
||||
(t/use-fixtures :each thi/test-fixture)
|
||||
|
||||
;; ---- Helpers
|
||||
|
||||
(defn- add-flex-frame
|
||||
"Create a flex layout frame"
|
||||
[file frame-label & {:keys [width height] :as params}]
|
||||
(ths/add-sample-shape file frame-label
|
||||
(merge {:type :frame
|
||||
:name "FlexFrame"
|
||||
:layout-flex-dir :row
|
||||
:width (or width 200)
|
||||
:height (or height 200)}
|
||||
params)))
|
||||
|
||||
(defn- add-rect-child
|
||||
"Create a rectangle child inside a parent"
|
||||
[file rect-label parent-label & {:keys [width height x y] :as params}]
|
||||
(ths/add-sample-shape file rect-label
|
||||
(merge {:type :rect
|
||||
:name "Rect"
|
||||
:parent-label parent-label
|
||||
:width (or width 50)
|
||||
:height (or height 50)
|
||||
:x (or x 0)
|
||||
:y (or y 0)}
|
||||
params)))
|
||||
|
||||
(defn- add-ghost-child-id
|
||||
"Add a non-existent child ID to a frame's shapes list.
|
||||
This simulates data inconsistency where a child ID is referenced
|
||||
but the child shape doesn't exist in objects."
|
||||
[file frame-label ghost-id]
|
||||
(let [page (thf/current-page file)
|
||||
frame-id (thi/id frame-label)]
|
||||
(update file :data
|
||||
(fn [file-data]
|
||||
(update-in file-data [:pages-index (:id page) :objects frame-id :shapes]
|
||||
conj ghost-id)))))
|
||||
|
||||
;; ---- Tests
|
||||
|
||||
(t/deftest flex-layout-with-normal-children
|
||||
(t/testing "set-objects-modifiers processes flex layout children correctly"
|
||||
(let [file (-> (thf/sample-file :file1)
|
||||
(add-flex-frame :frame1)
|
||||
(add-rect-child :rect1 :frame1))
|
||||
page (thf/current-page file)
|
||||
objects (:objects page)
|
||||
frame-id (thi/id :frame1)
|
||||
rect-id (thi/id :rect1)
|
||||
|
||||
;; Create a move modifier for the rectangle
|
||||
modif-tree {rect-id {:modifiers (ctm/move-modifiers (gpt/point 10 20))}}
|
||||
|
||||
;; This should not crash
|
||||
result (gm/set-objects-modifiers modif-tree objects)]
|
||||
|
||||
(t/is (some? result))
|
||||
;; The rectangle should have modifiers
|
||||
(t/is (contains? result rect-id)))))
|
||||
|
||||
(t/deftest flex-layout-with-nonexistent-child
|
||||
(t/testing "set-objects-modifiers handles flex frame with non-existent child in shapes"
|
||||
(let [ghost-id (thi/next-uuid)
|
||||
file (-> (thf/sample-file :file1)
|
||||
(add-flex-frame :frame1)
|
||||
(add-rect-child :rect1 :frame1)
|
||||
;; Add a non-existent child ID to the frame's shapes
|
||||
(add-ghost-child-id :frame1 ghost-id))
|
||||
page (thf/current-page file)
|
||||
objects (:objects page)
|
||||
rect-id (thi/id :rect1)
|
||||
|
||||
;; Create a move modifier for the existing rectangle
|
||||
modif-tree {rect-id {:modifiers (ctm/move-modifiers (gpt/point 10 20))}}
|
||||
|
||||
;; This should NOT crash even though the flex frame has
|
||||
;; a child ID (ghost-id) that doesn't exist in objects
|
||||
result (gm/set-objects-modifiers modif-tree objects)]
|
||||
|
||||
(t/is (some? result))
|
||||
(t/is (contains? result rect-id)))))
|
||||
|
||||
(t/deftest flex-layout-with-all-ghost-children
|
||||
(t/testing "set-objects-modifiers handles flex frame with only non-existent children"
|
||||
(let [ghost1 (thi/next-uuid)
|
||||
ghost2 (thi/next-uuid)
|
||||
file (-> (thf/sample-file :file1)
|
||||
(add-flex-frame :frame1)
|
||||
;; Add only non-existent children to the frame's shapes
|
||||
(add-ghost-child-id :frame1 ghost1)
|
||||
(add-ghost-child-id :frame1 ghost2))
|
||||
page (thf/current-page file)
|
||||
objects (:objects page)
|
||||
frame-id (thi/id :frame1)
|
||||
|
||||
;; Create a move modifier for the frame itself
|
||||
modif-tree {frame-id {:modifiers (ctm/move-modifiers (gpt/point 5 5))}}
|
||||
|
||||
;; Should not crash even though the flex frame has
|
||||
;; no existing children in its shapes list
|
||||
result (gm/set-objects-modifiers modif-tree objects)]
|
||||
|
||||
(t/is (some? result)))))
|
||||
|
||||
(t/deftest grid-layout-with-nonexistent-child
|
||||
(t/testing "set-objects-modifiers handles grid frame with non-existent child in shapes"
|
||||
(let [ghost-id (thi/next-uuid)
|
||||
file (-> (thf/sample-file :file1)
|
||||
(ths/add-sample-shape :frame1
|
||||
{:type :frame
|
||||
:name "GridFrame"
|
||||
:layout-grid-dir :row
|
||||
:width 200
|
||||
:height 200})
|
||||
(add-rect-child :rect1 :frame1)
|
||||
(add-ghost-child-id :frame1 ghost-id))
|
||||
page (thf/current-page file)
|
||||
objects (:objects page)
|
||||
rect-id (thi/id :rect1)
|
||||
|
||||
modif-tree {rect-id {:modifiers (ctm/move-modifiers (gpt/point 10 20))}}
|
||||
|
||||
;; Should not crash for grid layout with ghost child
|
||||
result (gm/set-objects-modifiers modif-tree objects)]
|
||||
|
||||
(t/is (some? result))
|
||||
(t/is (contains? result rect-id)))))
|
||||
|
||||
(t/deftest flex-layout-resize-with-nonexistent-child
|
||||
(t/testing "resize modifier propagation handles non-existent children"
|
||||
(let [ghost-id (thi/next-uuid)
|
||||
file (-> (thf/sample-file :file1)
|
||||
(add-flex-frame :frame1)
|
||||
(add-rect-child :rect1 :frame1)
|
||||
(add-ghost-child-id :frame1 ghost-id))
|
||||
page (thf/current-page file)
|
||||
objects (:objects page)
|
||||
frame-id (thi/id :frame1)
|
||||
|
||||
;; Create a resize modifier for the frame itself
|
||||
modif-tree {frame-id {:modifiers (ctm/resize-modifiers
|
||||
(gpt/point 2 2)
|
||||
(gpt/point 0 0))}}
|
||||
|
||||
;; Should not crash when propagating resize through flex layout
|
||||
;; that has ghost children
|
||||
result (gm/set-objects-modifiers modif-tree objects)]
|
||||
|
||||
(t/is (some? result))
|
||||
;; The frame should have modifiers
|
||||
(t/is (contains? result frame-id)))))
|
||||
|
||||
(t/deftest nested-flex-layout-with-nonexistent-child
|
||||
(t/testing "nested flex layout handles non-existent children in outer frame"
|
||||
(let [ghost-id (thi/next-uuid)
|
||||
file (-> (thf/sample-file :file1)
|
||||
(add-flex-frame :outer-frame)
|
||||
(add-flex-frame :inner-frame :parent-label :outer-frame)
|
||||
(add-rect-child :rect1 :inner-frame)
|
||||
(add-ghost-child-id :outer-frame ghost-id))
|
||||
page (thf/current-page file)
|
||||
objects (:objects page)
|
||||
rect-id (thi/id :rect1)
|
||||
|
||||
modif-tree {rect-id {:modifiers (ctm/move-modifiers (gpt/point 5 10))}}
|
||||
|
||||
result (gm/set-objects-modifiers modif-tree objects)]
|
||||
|
||||
(t/is (some? result))
|
||||
(t/is (contains? result rect-id)))))
|
||||
@@ -12,6 +12,7 @@
|
||||
[common-tests.data-test]
|
||||
[common-tests.files-changes-test]
|
||||
[common-tests.files-migrations-test]
|
||||
[common-tests.geom-modifiers-test]
|
||||
[common-tests.geom-point-test]
|
||||
[common-tests.geom-shapes-test]
|
||||
[common-tests.geom-test]
|
||||
@@ -66,6 +67,7 @@
|
||||
'common-tests.data-test
|
||||
'common-tests.files-changes-test
|
||||
'common-tests.files-migrations-test
|
||||
'common-tests.geom-modifiers-test
|
||||
'common-tests.geom-point-test
|
||||
'common-tests.geom-shapes-test
|
||||
'common-tests.geom-test
|
||||
|
||||
Reference in New Issue
Block a user