🐛 Allow import to continue from recoverable failures

CHANGES.md

@@ -37,6 +37,7 @@
 - Fix dotted style in strokes [Taiga #2312](https://tree.taiga.io/project/penpot/issue/2312)
 - Fix problem when resizing texts inside groups [Taiga #2310](https://tree.taiga.io/project/penpot/issue/2310)
 - Fix problem with multiple exports [Taiga #2468](https://tree.taiga.io/project/penpot/issue/2468)
+- Allow import to continue from recoverable failures [#1412](https://github.com/penpot/penpot/issues/1412)
 
 ### :arrow_up: Deps updates
 ### :heart: Community contributions by (Thank you!)

common/src/app/common/file_builder.cljc

@@ -12,7 +12,6 @@
    [app.common.geom.shapes :as gsh]
    [app.common.pages.changes :as ch]
    [app.common.pages.init :as init]
-   [app.common.pages.spec :as spec]
    [app.common.spec :as us]
    [app.common.uuid :as uuid]
    [cuerdas.core :as str]))
@@ -21,15 +20,14 @@
 (def conjv (fnil conj []))
 (def conjs (fnil conj #{}))
 
-;; This flag controls if we should execute spec validation after every commit
-(def verify-on-commit? true)
-
 (defn- commit-change
   ([file change]
    (commit-change file change nil))
 
-  ([file change {:keys [add-container?]
+  ([file change {:keys [add-container?
-                 :or   {add-container? false}}]
+                        fail-on-spec?]
+                 :or   {add-container? false
+                        fail-on-spec? false}}]
    (let [component-id (:current-component-id file)
          change (cond-> change
                   (and add-container? (some? component-id))
@@ -39,11 +37,20 @@
                   (assoc :page-id  (:current-page-id file)
                          :frame-id (:current-frame-id file)))]
 
-     (when verify-on-commit?
+     (when fail-on-spec?
-       (us/assert ::spec/change change))
+       (us/verify :app.common.pages.spec/change change))
-     (-> file
+
-         (update :changes conjv change)
+     (let [valid? (us/valid? :app.common.pages.spec/change change)]
-         (update :data ch/process-changes [change] verify-on-commit?)))))
+       #?(:cljs
+          (when-not valid? (.warn js/console "Invalid shape" (clj->js change))))
+
+       (cond-> file
+         valid?
+         (-> (update :changes conjv change)
+             (update :data ch/process-changes [change] false))
+
+         (not valid?)
+         (update :errors conjv change))))))
 
 (defn- lookup-objects
   ([file]
@@ -56,15 +63,16 @@
       (get shape-id)))
 
 (defn- commit-shape [file obj]
-  (let [parent-id (-> file :parent-stack peek)]
+  (let [parent-id (-> file :parent-stack peek)
-    (-> file
+        change {:type :add-obj
-        (commit-change
+                :id (:id obj)
-         {:type :add-obj
+                :obj obj
-          :id (:id obj)
+                :parent-id parent-id}
-          :obj obj
-          :parent-id parent-id}
 
-         {:add-container? true}))))
+        fail-on-spec? (or (= :group (:type obj))
+                          (= :frame (:type obj)))]
+
+    (commit-change file change {:add-container? true :fail-on-spec? fail-on-spec?})))
 
 (defn setup-rect-selrect [obj]
   (let [rect      (select-keys obj [:x :y :width :height])
@@ -421,35 +429,36 @@
 (defn add-interaction
   [file from-id interaction-src]
 
-  (assert (some? (lookup-shape file from-id)) (str "Cannot locate shape with id " from-id))
+  (let [shape (lookup-shape file from-id)]
+    (if (nil? shape)
+      file
+      (let [{:keys [event-type action-type]} (read-classifier interaction-src)
+            {:keys [delay]} (read-event-opts interaction-src)
+            {:keys [destination overlay-pos-type overlay-position url
+                    close-click-outside background-overlay preserve-scroll]}
+            (read-action-opts interaction-src)
 
-  (let [{:keys [event-type action-type]} (read-classifier interaction-src)
+            interactions (-> shape
-        {:keys [delay]} (read-event-opts interaction-src)
+                             :interactions
-        {:keys [destination overlay-pos-type overlay-position url
+                             (conjv
-                close-click-outside background-overlay preserve-scroll]}
+                              (d/without-nils {:event-type event-type
-        (read-action-opts interaction-src)
+                                               :action-type action-type
+                                               :delay delay
+                                               :destination destination
+                                               :overlay-pos-type overlay-pos-type
+                                               :overlay-position overlay-position
+                                               :url url
+                                               :close-click-outside close-click-outside
+                                               :background-overlay background-overlay
+                                               :preserve-scroll preserve-scroll})))]
+        (commit-change
+         file
+         {:type :mod-obj
+          :page-id (:current-page-id file)
+          :id from-id
 
-        interactions (-> (lookup-shape file from-id)
+          :operations
-                         :interactions
+          [{:type :set :attr :interactions :val interactions}]})))))
-                         (conjv
-                          (d/without-nils {:event-type event-type
-                                           :action-type action-type
-                                           :delay delay
-                                           :destination destination
-                                           :overlay-pos-type overlay-pos-type
-                                           :overlay-position overlay-position
-                                           :url url
-                                           :close-click-outside close-click-outside
-                                           :background-overlay background-overlay
-                                           :preserve-scroll preserve-scroll})))]
-    (commit-change
-     file
-     {:type :mod-obj
-      :page-id (:current-page-id file)
-      :id from-id
-
-      :operations
-      [{:type :set :attr :interactions :val interactions}]})))
 
 (defn generate-changes
   [file]

common/src/app/common/spec.cljc

@@ -31,6 +31,8 @@
 (def max-safe-int (int 1e6))
 (def min-safe-int (int -1e6))
 
+(def valid? s/valid?)
+
 ;; --- Conformers
 
 (defn uuid-conformer
@@ -216,8 +218,11 @@
                 :code :spec-validation
                 :hint hint
                 :ctx  ctx
+                :value val
                 ::s/problems (::s/problems data)))))
 
+
+
 (defmacro assert
   "Development only assertion macro."
   [spec x]

frontend/resources/styles/main/partials/modal.scss

@@ -322,6 +322,10 @@
       fill: $color-success;
     }
 
+    .icon-msg-warning {
+      fill: $color-warning;
+    }
+
     .icon-close {
       transform: rotate(45deg);
       fill: $color-danger;
@@ -392,6 +396,13 @@
         fill: $color-white;
       }
     }
+
+    &.warning {
+      background: $color-warning-lighter;
+      .icon {
+        background: $color-warning;
+      }
+    }
   }
 
   .error-message {

frontend/src/app/main/ui/dashboard/import.cljs

@@ -98,7 +98,7 @@
        (filter #(= :ready (:status %)))
        (mapv #(assoc % :status :importing))))
 
-(defn update-status [files file-id status progress]
+(defn update-status [files file-id status progress errors]
   (->> files
        (mapv (fn [file]
                (cond-> file
@@ -106,7 +106,10 @@
                  (assoc :status status)
 
                  (and (= file-id (:file-id file)) (= status :import-progress))
-                 (assoc :progress progress))))))
+                 (assoc :progress progress)
+
+                 (= file-id (:file-id file))
+                 (assoc :errors errors))))))
 
 (defn parse-progress-message
   [message]
@@ -139,9 +142,10 @@
 
   (let [loading?       (or (= :analyzing (:status file))
                            (= :importing (:status file)))
-        load-success?  (= :import-success (:status file))
         analyze-error? (= :analyze-error (:status file))
+        import-finish? (= :import-finish (:status file))
         import-error?  (= :import-error (:status file))
+        import-warn?   (d/not-empty? (:errors file))
         ready?         (= :ready (:status file))
         is-shared?     (:shared file)
         progress       (:progress file)
@@ -177,7 +181,8 @@
     [:div.file-entry
      {:class (dom/classnames
               :loading  loading?
-              :success  load-success?
+              :success  (and import-finish? (not import-warn?) (not import-error?))
+              :warning  (and import-finish? import-warn? (not import-error?))
               :error    (or import-error? analyze-error?)
               :editable (and ready? (not editing?)))}
 
@@ -185,8 +190,9 @@
       [:div.file-icon
        (cond loading?       i/loader-pencil
              ready?         i/logo-icon
-             load-success?  i/tick
+             import-warn?   i/msg-warning
              import-error?  i/close
+             import-finish? i/tick
              analyze-error? i/close)]
 
       (if editing?
@@ -212,7 +218,7 @@
        [:div.error-message
         (tr "dashboard.import.import-error")]
 
-       (and (not load-success?) (some? progress))
+       (and (not import-finish?) (some? progress))
        [:div.progress-message (parse-progress-message progress)])
 
      [:div.linked-libraries
@@ -258,9 +264,8 @@
                   :project-id project-id
                   :files files})
                 (rx/subs
-                 (fn [{:keys [file-id status message] :as msg}]
+                 (fn [{:keys [file-id status message errors] :as msg}]
-                   (log/debug :msg msg)
+                   (swap! state update :files update-status file-id status message errors))))))
-                   (swap! state update :files update-status file-id status message))))))
 
         handle-cancel
         (mf/use-callback
@@ -291,7 +296,8 @@
            (st/emit! (modal/hide))
            (when on-finish-import (on-finish-import))))
 
-        success-files (->> @state :files (filter #(= (:status %) :import-success)) count)
+        warning-files (->> @state :files (filter #(and (= (:status %) :import-finish) (d/not-empty? (:errors %)))) count)
+        success-files (->> @state :files (filter #(and (= (:status %) :import-finish) (empty? (:errors %)))) count)
         pending-analysis? (> (->> @state :files (filter #(= (:status %) :analyzing)) count) 0)
         pending-import? (> (->> @state :files (filter #(= (:status %) :importing)) count) 0)]
 
@@ -316,11 +322,15 @@
         {:on-click handle-cancel} i/close]]
 
       [:div.modal-content
-       (when (and (= :importing (:status @state))
+       (when (and (= :importing (:status @state)) (not pending-import?))
-                  (not pending-import?))
+         (if (> warning-files 0)
-         [:div.feedback-banner
+           [:div.feedback-banner.warning
-          [:div.icon i/checkbox-checked]
+            [:div.icon i/msg-warning]
-          [:div.message (tr "dashboard.import.import-message" success-files)]])
+            [:div.message (tr "dashboard.import.import-warning" warning-files success-files)]]
+
+           [:div.feedback-banner
+            [:div.icon i/checkbox-checked]
+            [:div.message (tr "dashboard.import.import-message" success-files)]]))
 
        (for [file (->> (:files @state) (filterv (comp not :deleted?)))]
          (let [editing?      (and (some? (:file-id file))

frontend/src/app/worker/import.cljs

@@ -176,7 +176,9 @@
           (rx/tap #(reset! revn (:revn %)))
           (rx/ignore))
 
-     (rp/mutation :persist-temp-file {:id file-id}))))
+     (->> (rp/mutation :persist-temp-file {:id file-id})
+          ;; We use merge to keep some information not stored in back-end
+          (rx/map #(merge file %))))))
 
 (defn upload-media-files
   "Upload a image to the backend and returns its id"
@@ -457,8 +459,7 @@
 
   (let [progress-str (rx/subject)
         context (assoc context :progress progress-str)]
-    (rx/merge
+    [progress-str
-     progress-str
      (->> (rx/of file)
           (rx/flat-map (partial process-pages context))
           (rx/tap #(progress! context :process-colors))
@@ -470,7 +471,7 @@
           (rx/tap #(progress! context :process-components))
           (rx/flat-map (partial process-library-components context))
           (rx/flat-map (partial send-changes context))
-          (rx/tap #(rx/end! progress-str))))))
+          (rx/tap #(rx/end! progress-str)))]))
 
 (defn create-files
   [context files]
@@ -482,7 +483,6 @@
           (rx/flat-map
            (fn [context]
              (->> (create-file context)
-                  (rx/tap #(.log js/console "create-file" (clj->js %)))
                   (rx/map #(vector % (first (get data (:file-id context)))))))))
 
      (->> (rx/from files)
@@ -509,20 +509,29 @@
   (let [context {:project-id project-id
                  :resolve    (resolve-factory)}]
     (->> (create-files context files)
-         (rx/catch #(.error js/console "IMPORT ERROR" %))
+         (rx/catch #(.error js/console "IMPORT ERROR" (clj->js %)))
          (rx/flat-map
           (fn [[file data]]
             (->> (rx/concat
                   (->> (uz/load-from-url (:uri data))
                        (rx/map #(-> context (assoc :zip %) (merge data)))
-                       (rx/flat-map #(process-file % file)))
+                       (rx/flat-map
-                  (rx/of
+                        (fn [context]
-                   {:status :import-success
+                          ;; process file retrieves a stream that will emit progress notifications
-                    :file-id (:file-id data)}))
+                          ;; and other that will emit the files once imported
+                          (let [[progress-stream file-stream] (process-file context file)]
+                            (rx/merge
+                             progress-stream
+                             (->> file-stream
+                                  (rx/map
+                                   (fn [file]
+                                     {:status :import-finish
+                                      :errors (:errors file)
+                                      :file-id (:file-id data)})))))))))
 
                  (rx/catch
                      (fn [err]
-                       (.error js/console "ERROR" (:file-id data) err)
+                       (.error js/console "ERROR" (str (:file-id data)) (clj->js err) (clj->js (.-data err)))
                        (rx/of {:status :import-error
                                :file-id (:file-id data)
                                :error (.-message err)

frontend/translations/en.po

@@ -3322,3 +3322,6 @@ msgstr "Insufficient members to leave team, you probably want to delete it."
 
 msgid "errors.auth.unable-to-login"
 msgstr "Looks like you are not authenticated or session expired."
+
+msgid "dashboard.import.import-warning"
+msgstr "Some files containted invalid objects that have been removed."

frontend/translations/es.po

@@ -3309,3 +3309,6 @@ msgstr "Actualizar"
 
 msgid "workspace.viewport.click-to-close-path"
 msgstr "Pulsar para cerrar la ruta"
+
+msgid "dashboard.import.import-warning"
+msgstr "Algunos ficheros contenían objetos erroneos que no han sido importados."