From 9c2a80bfa105271487d9e09bdf3a6d4bd42e4dce Mon Sep 17 00:00:00 2001 From: FairyPiggyDev Date: Thu, 30 Apr 2026 09:54:24 -0400 Subject: [PATCH] :bug: Fix crash pasting component with variants from shared library (#9136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Andrey Antukh --- CHANGES.md | 1 + common/src/app/common/logic/libraries.cljc | 57 ++++++++++++------- .../logic/comp_creation_test.cljc | 56 ++++++++++++++++++ 3 files changed, 93 insertions(+), 21 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e65534835f..ddb850f0b0 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -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 `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 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) - 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) diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index 35df16aa86..d1d03f68aa 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/common/src/app/common/logic/libraries.cljc @@ -118,8 +118,9 @@ (defn- duplicate-component "Clone the root shape of the component and all children. Generate new - ids from all of them." - [component new-component-id library-data force-id delta variant-id] + ids from all of them. Optionally set the component-file if the file where the + 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) main-instance-shape (ctf/get-component-root library-data component) delta (or delta (gpt/point (+ (:width main-instance-shape) 50) 0)) @@ -141,10 +142,18 @@ update-new-shape (fn [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)) (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 (and (ctk/instance-root? new-shape) (some? variant-id)) (assoc :variant-id variant-id) @@ -188,7 +197,7 @@ (defn generate-duplicate-component "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) new-name (:name component) @@ -197,7 +206,7 @@ target-page-id (or page-id (:id main-instance-page)) [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 (-> changes @@ -2727,7 +2736,7 @@ frames))) (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)) objects (:objects component-page) component-shape (get objects (:main-instance-id component)) @@ -2741,7 +2750,8 @@ {:apply-changes-local-library? true :delta delta :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? (str ctv/value-prefix (-> (cfv/extract-properties-values (:data library) objects (:id parent)) @@ -2764,15 +2774,18 @@ (defn generate-duplicate-component-change - [changes objects page main parent-id frame-id delta libraries library-data ids-map] - (let [main-id (:id main) - component-id (:component-id main) - file-id (:component-file main) - component (ctf/get-component libraries file-id component-id) - pos (as-> (gsh/move main delta) $ - (gpt/point (:x $) (:y $))) + [changes objects page main parent-id frame-id delta libraries library-data ids-map & {:keys [new-component-file]}] + (let [main-id (:id main) + component-id (:component-id main) + ;; Source library file id (where the component was originally + ;; defined). Renamed from `file-id` to make the contrast with + ;; `new-component-file` explicit when duplicating across files. + 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 @@ -2799,25 +2812,27 @@ (and (ctk/is-variant? main) in-variant-container?) (duplicate-variant changes - (get libraries file-id) + (get libraries source-file-id) component pos parent (:id page) - false) + false + new-component-file) (ctk/is-variant-container? parent) (duplicate-variant changes - (get libraries file-id) + (get libraries source-file-id) component pos parent (:id page) - true) + true + new-component-file) :else (generate-instantiate-component changes objects - file-id + source-file-id component-id pos page @@ -2841,7 +2856,7 @@ changes (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 (let [frame? (cfh/frame-shape? obj) diff --git a/common/test/common_tests/logic/comp_creation_test.cljc b/common/test/common_tests/logic/comp_creation_test.cljc index 462734d6ee..ad1277879b 100644 --- a/common/test/common_tests/logic/comp_creation_test.cljc +++ b/common/test/common_tests/logic/comp_creation_test.cljc @@ -304,6 +304,62 @@ (t/is (= (thi/id :main1-child) (:id child1'))) (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 (let [;; ==== Setup file (-> (thf/sample-file :file1)