mirror of
https://github.com/penpot/penpot.git
synced 2026-07-28 00:48:22 +00:00
🐛 Fix crash pasting component with variants from shared library (#9136)
Copying a component with variants from a shared library file ("Lib")
and pasting it into a file that uses that library ("Using Lib") would
crash the destination file with the referential-integrity validator
error:
{:code :component-main-external
:hint "Main instance should refer to a component in the same file"}
Root cause
----------
Paste goes through `generate-duplicate-shape-change` in
`common/src/app/common/logic/libraries.cljc`. When the shape is a
main instance of a known component and the copy set includes its
variant container, dispatch lands in `duplicate-variant`, then
`generate-duplicate-component`, and finally `duplicate-component`,
which clones the main-instance shape tree. Its `update-new-shape`
helper already re-links the new outer main's `:component-id` to the
freshly created local component (`new-component-id`), but it never
touches `:component-file`. The cloned shape therefore inherits
`:component-file` from the source library while the new component is
registered in the destination's local library
(`:apply-changes-local-library? true`), leaving the main-instance
dangling.
Fix
---
Extend `update-new-shape` with a second clause, sibling to the
existing `:component-id` rewrite: when a destination file id is
provided and differs from the new main's current `:component-file`,
re-root the shape. The same `(= (:component-id new-shape) (:id
component))` guard already used for the id rewrite ensures only the
outer main-instance is touched; nested shapes are unaffected.
The destination file id is threaded from the paste entry point
through the two orchestration functions that already knew the
source/destination distinction:
- `generate-duplicate-shape-change` — supplies the destination
`file-id` it already has in scope when dispatching to
`generate-duplicate-component-change`.
- `generate-duplicate-component-change` — accepts `:new-component-file`
as a kwarg; renames its internal `file-id` binding to
`source-file-id` for clarity (it was always the component's
originating library file); forwards `new-component-file` to
`duplicate-variant`.
- `duplicate-variant` — takes and forwards the `new-component-file`
positional arg.
- `generate-duplicate-component` — accepts `:new-component-file` kwarg
and passes it to `duplicate-component`.
- `duplicate-component` — applies the rewrite inside
`update-new-shape`. The `new-component-file` parameter is placed
right after `new-component-id` since component-id and component-file
are typically managed together.
Same-file duplication is not affected: without `:new-component-file`
the new clause is skipped, and when source and destination match the
`(not= new-component-file (:component-file new-shape))` guard fails.
Tests
-----
Added in `common/test/common_tests/logic/comp_creation_test.cljc`:
- `test-duplicate-component-rewrites-component-file-to-destination`
asserts that passing `:new-component-file` to
`generate-duplicate-component` produces a main-instance with
`:component-file` equal to the destination id.
- `test-duplicate-component-keeps-component-file-without-dest`
baseline: without `:new-component-file`, `:component-file` is left
untouched, matching pre-existing same-file behavior.
Github #8144
Signed-off-by: FairyPigDev <luislee3108@gmail.com>
Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
parent
76c1b9afab
commit
9c2a80bfa1
@ -69,6 +69,7 @@
|
|||||||
- Fix Plugin API `shape.applyToken()` / `token.applyToShapes()` / `token.applyToSelected()` rejecting JS-array attribute lists like `["fill"]`: switched the inner schemas to `[::sm/set ...]` (which has the JS array → Clojure set decoder) and made `token-attr-plugin->token-attr` accept string inputs by coercing them to keywords before consulting the alias map [Github #9162](https://github.com/penpot/penpot/issues/9162)
|
- Fix Plugin API `shape.applyToken()` / `token.applyToShapes()` / `token.applyToSelected()` rejecting JS-array attribute lists like `["fill"]`: switched the inner schemas to `[::sm/set ...]` (which has the JS array → Clojure set decoder) and made `token-attr-plugin->token-attr` accept string inputs by coercing them to keywords before consulting the alias map [Github #9162](https://github.com/penpot/penpot/issues/9162)
|
||||||
- Fix `PENPOT_OIDC_USER_INFO_SOURCE` flag being silently ignored (`userinfo` / `token`) in the OIDC callback, causing "incomplete user info" failures during registration [Github #9108](https://github.com/penpot/penpot/issues/9108)
|
- Fix `PENPOT_OIDC_USER_INFO_SOURCE` flag being silently ignored (`userinfo` / `token`) in the OIDC callback, causing "incomplete user info" failures during registration [Github #9108](https://github.com/penpot/penpot/issues/9108)
|
||||||
- Fix `get-view-only-bundle` crashing when a share-link viewer encounters a team member whose email lacks `@` (NullPointerException in `obfuscate-email`) or whose domain has no `.` (previously produced a dangling-dot `****@****.`); now the viewer-side obfuscation is nil-safe and omits the trailing dot when the domain has no TLD
|
- Fix `get-view-only-bundle` crashing when a share-link viewer encounters a team member whose email lacks `@` (NullPointerException in `obfuscate-email`) or whose domain has no `.` (previously produced a dangling-dot `****@****.`); now the viewer-side obfuscation is nil-safe and omits the trailing dot when the domain has no TLD
|
||||||
|
- Fix crash when pasting a component with variants from an external shared library into a file that uses that library (by @FairyPigDev) [Github #8144](https://github.com/penpot/penpot/issues/8144)
|
||||||
- Remove `corepack` from the MCP local launcher so it runs on Node.js 25+, where corepack is no longer bundled [Github #8877](https://github.com/penpot/penpot/issues/8877)
|
- Remove `corepack` from the MCP local launcher so it runs on Node.js 25+, where corepack is no longer bundled [Github #8877](https://github.com/penpot/penpot/issues/8877)
|
||||||
- Fix Copy as SVG: emit a single valid SVG document when multiple shapes are selected, and publish `image/svg+xml` to the clipboard so the paste target works in Inkscape and other SVG-native tools [Github #838](https://github.com/penpot/penpot/issues/838)
|
- Fix Copy as SVG: emit a single valid SVG document when multiple shapes are selected, and publish `image/svg+xml` to the clipboard so the paste target works in Inkscape and other SVG-native tools [Github #838](https://github.com/penpot/penpot/issues/838)
|
||||||
- Reset profile submenu state when the account menu closes (by @eureka0928) [Github #8947](https://github.com/penpot/penpot/issues/8947)
|
- Reset profile submenu state when the account menu closes (by @eureka0928) [Github #8947](https://github.com/penpot/penpot/issues/8947)
|
||||||
|
|||||||
@ -118,8 +118,9 @@
|
|||||||
|
|
||||||
(defn- duplicate-component
|
(defn- duplicate-component
|
||||||
"Clone the root shape of the component and all children. Generate new
|
"Clone the root shape of the component and all children. Generate new
|
||||||
ids from all of them."
|
ids from all of them. Optionally set the component-file if the file where the
|
||||||
[component new-component-id library-data force-id delta variant-id]
|
new component will reside is different than the origin one."
|
||||||
|
[component new-component-id new-component-file library-data force-id delta variant-id]
|
||||||
(let [main-instance-page (ctf/get-component-page library-data component)
|
(let [main-instance-page (ctf/get-component-page library-data component)
|
||||||
main-instance-shape (ctf/get-component-root library-data component)
|
main-instance-shape (ctf/get-component-root library-data component)
|
||||||
delta (or delta (gpt/point (+ (:width main-instance-shape) 50) 0))
|
delta (or delta (gpt/point (+ (:width main-instance-shape) 50) 0))
|
||||||
@ -141,10 +142,18 @@
|
|||||||
update-new-shape
|
update-new-shape
|
||||||
(fn [new-shape _]
|
(fn [new-shape _]
|
||||||
(cond-> new-shape
|
(cond-> new-shape
|
||||||
; Link the new main to the new component
|
;; Link the new main to the new component, and re-root it
|
||||||
|
;; to the destination file when duplicating across files.
|
||||||
|
;; Only the outer main matches `(:id component)`, so
|
||||||
|
;; nested main-instances are not touched here.
|
||||||
(= (:component-id new-shape) (:id component))
|
(= (:component-id new-shape) (:id component))
|
||||||
(assoc :component-id new-component-id)
|
(assoc :component-id new-component-id)
|
||||||
|
|
||||||
|
(and (= (:component-id new-shape) (:id component))
|
||||||
|
(some? new-component-file)
|
||||||
|
(not= new-component-file (:component-file new-shape)))
|
||||||
|
(assoc :component-file new-component-file)
|
||||||
|
|
||||||
; If it is the instance root, add it the variant-id
|
; If it is the instance root, add it the variant-id
|
||||||
(and (ctk/instance-root? new-shape) (some? variant-id))
|
(and (ctk/instance-root? new-shape) (some? variant-id))
|
||||||
(assoc :variant-id variant-id)
|
(assoc :variant-id variant-id)
|
||||||
@ -188,7 +197,7 @@
|
|||||||
|
|
||||||
(defn generate-duplicate-component
|
(defn generate-duplicate-component
|
||||||
"Create a new component copied from the one with the given id."
|
"Create a new component copied from the one with the given id."
|
||||||
[changes library component-id new-component-id & {:keys [new-shape-id apply-changes-local-library? delta new-variant-id page-id]}]
|
[changes library component-id new-component-id & {:keys [new-component-file new-shape-id apply-changes-local-library? delta new-variant-id page-id]}]
|
||||||
(let [component (ctkl/get-component (:data library) component-id)
|
(let [component (ctkl/get-component (:data library) component-id)
|
||||||
new-name (:name component)
|
new-name (:name component)
|
||||||
|
|
||||||
@ -197,7 +206,7 @@
|
|||||||
target-page-id (or page-id (:id main-instance-page))
|
target-page-id (or page-id (:id main-instance-page))
|
||||||
|
|
||||||
[new-main-instance-shape new-main-instance-shapes]
|
[new-main-instance-shape new-main-instance-shapes]
|
||||||
(duplicate-component component new-component-id (:data library) new-shape-id delta new-variant-id)]
|
(duplicate-component component new-component-id new-component-file (:data library) new-shape-id delta new-variant-id)]
|
||||||
|
|
||||||
[new-main-instance-shape
|
[new-main-instance-shape
|
||||||
(-> changes
|
(-> changes
|
||||||
@ -2727,7 +2736,7 @@
|
|||||||
frames)))
|
frames)))
|
||||||
|
|
||||||
(defn- duplicate-variant
|
(defn- duplicate-variant
|
||||||
[changes library component base-pos parent page-id into-new-variant?]
|
[changes library component base-pos parent page-id into-new-variant? new-component-file]
|
||||||
(let [component-page (ctpl/get-page (:data library) (:main-instance-page component))
|
(let [component-page (ctpl/get-page (:data library) (:main-instance-page component))
|
||||||
objects (:objects component-page)
|
objects (:objects component-page)
|
||||||
component-shape (get objects (:main-instance-id component))
|
component-shape (get objects (:main-instance-id component))
|
||||||
@ -2741,7 +2750,8 @@
|
|||||||
{:apply-changes-local-library? true
|
{:apply-changes-local-library? true
|
||||||
:delta delta
|
:delta delta
|
||||||
:new-variant-id (if into-new-variant? nil (:id parent))
|
:new-variant-id (if into-new-variant? nil (:id parent))
|
||||||
:page-id page-id})
|
:page-id page-id
|
||||||
|
:new-component-file new-component-file})
|
||||||
value (when into-new-variant?
|
value (when into-new-variant?
|
||||||
(str ctv/value-prefix
|
(str ctv/value-prefix
|
||||||
(-> (cfv/extract-properties-values (:data library) objects (:id parent))
|
(-> (cfv/extract-properties-values (:data library) objects (:id parent))
|
||||||
@ -2764,15 +2774,18 @@
|
|||||||
|
|
||||||
|
|
||||||
(defn generate-duplicate-component-change
|
(defn generate-duplicate-component-change
|
||||||
[changes objects page main parent-id frame-id delta libraries library-data ids-map]
|
[changes objects page main parent-id frame-id delta libraries library-data ids-map & {:keys [new-component-file]}]
|
||||||
(let [main-id (:id main)
|
(let [main-id (:id main)
|
||||||
component-id (:component-id main)
|
component-id (:component-id main)
|
||||||
file-id (:component-file main)
|
;; Source library file id (where the component was originally
|
||||||
component (ctf/get-component libraries file-id component-id)
|
;; defined). Renamed from `file-id` to make the contrast with
|
||||||
pos (as-> (gsh/move main delta) $
|
;; `new-component-file` explicit when duplicating across files.
|
||||||
(gpt/point (:x $) (:y $)))
|
source-file-id (:component-file main)
|
||||||
|
component (ctf/get-component libraries source-file-id component-id)
|
||||||
|
pos (as-> (gsh/move main delta) $
|
||||||
|
(gpt/point (:x $) (:y $)))
|
||||||
|
|
||||||
parent (get objects parent-id)
|
parent (get objects parent-id)
|
||||||
|
|
||||||
|
|
||||||
;; When we duplicate a variant alone, we will instanciate it
|
;; When we duplicate a variant alone, we will instanciate it
|
||||||
@ -2799,25 +2812,27 @@
|
|||||||
|
|
||||||
(and (ctk/is-variant? main) in-variant-container?)
|
(and (ctk/is-variant? main) in-variant-container?)
|
||||||
(duplicate-variant changes
|
(duplicate-variant changes
|
||||||
(get libraries file-id)
|
(get libraries source-file-id)
|
||||||
component
|
component
|
||||||
pos
|
pos
|
||||||
parent
|
parent
|
||||||
(:id page)
|
(:id page)
|
||||||
false)
|
false
|
||||||
|
new-component-file)
|
||||||
|
|
||||||
(ctk/is-variant-container? parent)
|
(ctk/is-variant-container? parent)
|
||||||
(duplicate-variant changes
|
(duplicate-variant changes
|
||||||
(get libraries file-id)
|
(get libraries source-file-id)
|
||||||
component
|
component
|
||||||
pos
|
pos
|
||||||
parent
|
parent
|
||||||
(:id page)
|
(:id page)
|
||||||
true)
|
true
|
||||||
|
new-component-file)
|
||||||
:else
|
:else
|
||||||
(generate-instantiate-component changes
|
(generate-instantiate-component changes
|
||||||
objects
|
objects
|
||||||
file-id
|
source-file-id
|
||||||
component-id
|
component-id
|
||||||
pos
|
pos
|
||||||
page
|
page
|
||||||
@ -2841,7 +2856,7 @@
|
|||||||
changes
|
changes
|
||||||
|
|
||||||
(ctf/is-main-of-known-component? obj libraries)
|
(ctf/is-main-of-known-component? obj libraries)
|
||||||
(generate-duplicate-component-change changes objects page obj parent-id frame-id delta libraries library-data ids-map)
|
(generate-duplicate-component-change changes objects page obj parent-id frame-id delta libraries library-data ids-map {:new-component-file file-id})
|
||||||
|
|
||||||
:else
|
:else
|
||||||
(let [frame? (cfh/frame-shape? obj)
|
(let [frame? (cfh/frame-shape? obj)
|
||||||
|
|||||||
@ -304,6 +304,62 @@
|
|||||||
(t/is (= (thi/id :main1-child) (:id child1')))
|
(t/is (= (thi/id :main1-child) (:id child1')))
|
||||||
(t/is (not= (thi/id :main1-child) (:id child2')))))
|
(t/is (not= (thi/id :main1-child) (:id child2')))))
|
||||||
|
|
||||||
|
(t/deftest test-duplicate-component-rewrites-component-file-to-destination
|
||||||
|
;; Regression test for Issue #8144. When a component is duplicated
|
||||||
|
;; into a different file via `:apply-changes-local-library? true`
|
||||||
|
;; and `:new-component-file` is provided, the returned main-instance
|
||||||
|
;; shape must carry `:component-file` equal to the destination file
|
||||||
|
;; id so the referential-integrity validator
|
||||||
|
;; (:component-main-external) is satisfied.
|
||||||
|
(let [;; ==== Setup
|
||||||
|
file (-> (thf/sample-file :file1)
|
||||||
|
(tho/add-simple-component :component1
|
||||||
|
:main1-root
|
||||||
|
:main1-child))
|
||||||
|
|
||||||
|
component (thc/get-component file :component1)
|
||||||
|
new-component-file (uuid/next)
|
||||||
|
|
||||||
|
;; ==== Action
|
||||||
|
[new-shape _]
|
||||||
|
(cll/generate-duplicate-component (pcb/empty-changes)
|
||||||
|
file
|
||||||
|
(:id component)
|
||||||
|
(uuid/next)
|
||||||
|
{:apply-changes-local-library? true
|
||||||
|
:new-component-file new-component-file})]
|
||||||
|
|
||||||
|
;; ==== Check
|
||||||
|
(t/is (some? new-shape))
|
||||||
|
(t/is (ctk/main-instance? new-shape))
|
||||||
|
(t/is (= new-component-file (:component-file new-shape)))))
|
||||||
|
|
||||||
|
(t/deftest test-duplicate-component-keeps-component-file-without-dest
|
||||||
|
;; Baseline: when no `:new-component-file` is passed (same-file
|
||||||
|
;; duplication), the main-instance's `:component-file` is left
|
||||||
|
;; untouched, matching pre-existing behavior.
|
||||||
|
(let [;; ==== Setup
|
||||||
|
file (-> (thf/sample-file :file1)
|
||||||
|
(tho/add-simple-component :component1
|
||||||
|
:main1-root
|
||||||
|
:main1-child))
|
||||||
|
|
||||||
|
component (thc/get-component file :component1)
|
||||||
|
original-source (:component-file
|
||||||
|
(ths/get-shape-by-id file (:main-instance-id component)))
|
||||||
|
|
||||||
|
;; ==== Action
|
||||||
|
[new-shape _]
|
||||||
|
(cll/generate-duplicate-component (pcb/empty-changes)
|
||||||
|
file
|
||||||
|
(:id component)
|
||||||
|
(uuid/next)
|
||||||
|
{:apply-changes-local-library? true})]
|
||||||
|
|
||||||
|
;; ==== Check
|
||||||
|
(t/is (some? new-shape))
|
||||||
|
(t/is (= original-source (:component-file new-shape)))))
|
||||||
|
|
||||||
(t/deftest test-delete-component
|
(t/deftest test-delete-component
|
||||||
(let [;; ==== Setup
|
(let [;; ==== Setup
|
||||||
file (-> (thf/sample-file :file1)
|
file (-> (thf/sample-file :file1)
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user