diff --git a/frontend/src/app/main/data/exports/assets.cljs b/frontend/src/app/main/data/exports/assets.cljs index b1ced71ad6..d0cf4597e0 100644 --- a/frontend/src/app/main/data/exports/assets.cljs +++ b/frontend/src/app/main/data/exports/assets.cljs @@ -13,11 +13,14 @@ [app.main.data.exports.wasm :as wasm.exports] [app.main.data.helpers :as dsh] [app.main.data.modal :as modal] + [app.main.data.notifications :as ntf] [app.main.data.persistence :as dwp] + [app.main.errors :as errors] [app.main.features :as features] [app.main.repo :as rp] [app.main.store :as st] [app.util.dom :as dom] + [app.util.i18n :refer [tr]] [app.util.websocket :as ws] [beicon.v2.core :as rx] [cuerdas.core :as str] @@ -25,6 +28,37 @@ (def default-timeout 5000) +;; Longest wait for the pending changes of the exported files to be saved. +(def save-wait-timeout-ms 10000) + +(defn- export-file-ids + [exports] + (into #{} (keep :file-id) exports)) + +(defn- warn-unsaved-changes + "Warns that the export may miss the changes of `file-ids` not saved yet." + [file-ids] + (ptk/reify ::warn-unsaved-changes + ptk/WatchEvent + (watch [_ state _] + (when (dwp/unsaved-changes? state file-ids) + (rx/of (ntf/show {:content (tr "workspace.export.unsaved-changes") + :type :toast + :level :warning + :timeout 8000})))))) + +(defn- export-failed + "Shows an export failure as a toast, keeping the workspace open." + [cause] + (ptk/reify ::export-failed + ptk/EffectEvent + (effect [_ _ _] + (errors/flash :cause cause + :type (if (#{:server-error :internal} (:type (ex-data cause))) + :unhandled + :handled) + :hint (tr "errors.export-failed"))))) + (defn normalize-export [{:keys [object-id name] :as export}] (assoc export :name (if (str/blank? name) @@ -226,16 +260,16 @@ :wait true :is-wasm (wasm-export-enabled? state)})] (rx/concat - (dwp/force-persist-and-wait 400) + (dwp/force-persist-files-and-wait (export-file-ids [export]) save-wait-timeout-ms) + (rx/of (warn-unsaved-changes (export-file-ids [export]))) (->> (rp/cmd! :export params) (rx/map (fn [{:keys [filename mtype uri]}] (dom/trigger-download-uri filename mtype uri) (clear-export-state uuid/zero))) (rx/catch (fn [cause] - (rx/concat - (rx/of (clear-export-state uuid/zero)) - (rx/throw cause)))))))))))) + (rx/of (clear-export-state uuid/zero) + (export-failed cause)))))))))))) (defn request-multiple-export [{:keys [exports cmd name] @@ -246,6 +280,8 @@ ptk/WatchEvent (watch [_ state _] (let [resource-id (volatile! nil) + ;; Emits when the export request fails. + failed-s (rx/subject) profile-id (:profile-id state) ws-conn (:ws-conn state) params (cond-> @@ -263,46 +299,48 @@ (rx/map :payload) (rx/filter #(= :export-update (:type %))) (rx/filter #(= @resource-id (:resource-id %))) + (rx/take-until failed-s) (rx/share)) stopper (rx/filter #(or (= "ended" (:status %)) (= "error" (:status %))) - progress-stream)] + progress-stream) + + file-ids + (export-file-ids exports)] (swap! st/ongoing-tasks conj :export) - (rx/merge - ;; Force that all data is persisted; best effort. - (rx/of ::dwp/force-persist) + (rx/concat + (dwp/force-persist-files-and-wait file-ids save-wait-timeout-ms) + (rx/of (warn-unsaved-changes file-ids)) + (rx/merge + ;; Updates the export state with each progress message, until + ;; shortly after the export ends or fails. + (->> progress-stream + (rx/map update-export-status) + (rx/take-until (rx/delay 500 stopper)) + (rx/finalize (fn [] + (swap! st/ongoing-tasks disj :export)))) - ;; Launch the exportation process and stores the resource id - ;; locally. - (->> (rp/cmd! :export params) - (rx/map (fn [{:keys [id] :as resource}] - (vreset! resource-id id) - (initialize-export-status exports cmd resource)))) + ;; Clears the export state a while after the export ends. + (->> progress-stream + (rx/filter #(= "ended" (:status %))) + (rx/take 1) + (rx/delay default-timeout) + (rx/map #(clear-export-state @resource-id)) + (rx/take-until (rx/delay 6000 stopper))) - ;; We proceed to update the export state with incoming - ;; progress updates. We delay the stopper for give some time - ;; to update the status with ended or errored status before - ;; close the stream. - (->> progress-stream - (rx/map update-export-status) - (rx/take-until (rx/delay 500 stopper)) - (rx/finalize (fn [] - (swap! st/ongoing-tasks disj :export)))) - - ;; We hide need to hide the ui elements of the export after - ;; some interval. We also delay a little bit more the stopper - ;; for ensure that after some security time, the stream is - ;; completely closed. - (->> progress-stream - (rx/filter #(= "ended" (:status %))) - (rx/take 1) - (rx/delay default-timeout) - (rx/map #(clear-export-state @resource-id)) - (rx/take-until (rx/delay 6000 stopper))))))))) + ;; Sends the export request and keeps its resource id. It goes + ;; last so a failure reaches the progress listeners above. + (->> (rp/cmd! :export params) + (rx/map (fn [{:keys [id] :as resource}] + (vreset! resource-id id) + (initialize-export-status exports cmd resource))) + (rx/catch (fn [cause] + (rx/push! failed-s cause) + (rx/of (export-failed cause)))))))))))) (defn request-export [{:keys [exports] :as params}] diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index f7f7779478..82e9013cb5 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -109,6 +109,30 @@ ([timeout-ms] (rx/concat (rx/of ::force-persist) (wait-persisted timeout-ms)))) +(defn- files-queued? + "True when the queue in `pstate` still holds changes of any of `file-ids`." + [{:keys [queue index]} file-ids] + (boolean (some #(contains? file-ids (get-in index [% :file-id])) queue))) + +(defn unsaved-changes? + "True when any of `file-ids` has changes the server does not have yet." + [state file-ids] + (files-queued? (:persistence state) file-ids)) + +(defn force-persist-files-and-wait + "Sends the pending changes and waits until no change of `file-ids` is + queued, saving fails, or `timeout-ms` passes. Never raises; check + `unsaved-changes?` afterwards." + [file-ids timeout-ms] + (rx/concat + (rx/of ::force-persist) + (->> (rx/from-atom refs/persistence {:emit-current-value? true}) + (rx/filter #(or (= :error (:status %)) + (not (files-queued? % file-ids)))) + (rx/take 1) + (rx/ignore) + (rx/timeout timeout-ms (rx/empty))))) + (defn- next-status "Refuses downgrades: a save in progress stays :saving, a failed save stays :error until persistence is resumed, and a retrying episode stays diff --git a/frontend/test/frontend_tests/data/exports_assets_test.cljs b/frontend/test/frontend_tests/data/exports_assets_test.cljs index d4ae9edea8..0caae3d10e 100644 --- a/frontend/test/frontend_tests/data/exports_assets_test.cljs +++ b/frontend/test/frontend_tests/data/exports_assets_test.cljs @@ -9,12 +9,15 @@ [app.common.uuid :as uuid] [app.main.data.exports.assets :as de] [app.main.data.persistence :as dwp] + [app.main.errors :as errors] [app.main.repo :as repo] [app.main.store :as st] [app.util.dom :as dom] + [app.util.i18n :as i18n] [app.util.websocket :as ws] [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] [frontend-tests.helpers.events :as the] [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) @@ -55,7 +58,7 @@ (rx/of {:filename "export.png" :mtype "image/png" :uri "blob:export"}))) - dwp/force-persist-and-wait (mock/stub (fn [_] (rx/of ::force-persisted))) + dwp/force-persist-files-and-wait (mock/stub (fn [_ _] (rx/of ::force-persisted))) dom/trigger-download-uri (mock/stub (fn [& _] nil))} (fn [done'] (let [completed (fn [_state] @@ -97,3 +100,106 @@ (de/request-multiple-export {:exports exports}) :the/end))) done)))) + +(defn- export-store + "A store for the export tests that records store errors in `store-errors`." + [state store-errors] + (ptk/store {:state state :on-error #(swap! store-errors conj %)})) + +(defn- unsaved-state + "Test state whose save queue still holds a change of `file-id`." + [file-id] + (let [commit-id (uuid/next)] + (assoc (test-state) + :persistence {:queue (conj #queue [] commit-id) + :index {commit-id {:id commit-id :file-id file-id}}}))) + +;; Scenario: the exporter fails a single export. Proves: the user gets a +;; toast and stays in the workspace; the failure is not raised to the global +;; error handler, which would replace the workspace with an error page. +(t/deftest ^:async failed-simple-export-shows-a-toast-instead-of-raising + (let [store-errors (atom []) + flashed (atom []) + store (export-store (test-state) store-errors)] + (await + (mock/with-mocks* + {repo/cmd! (mock/stub (fn [_ _] + (rx/throw (ex-info "exporter failed" + {:type :server-error + :code :unhandled})))) + dwp/force-persist-files-and-wait (mock/stub (fn [_ _] (rx/empty))) + errors/flash (fn [& {:keys [hint type]}] + (swap! flashed conj [hint type])) + i18n/tr (mock/stub #(str "translated:" %))} + (ptk/emit! store (de/request-simple-export {:export export})) + (await (async/wait-for #(seq @flashed) "the failure is shown")) + (await (async/settle)) + (t/is (= [["translated:errors.export-failed" :unhandled]] @flashed) + "an exporter defect reports as unhandled") + (t/is (empty? @store-errors) "nothing reaches the global error handler") + (t/is (nil? (:export @store)) "the export state is cleared"))))) + +;; Scenario: the export request of a multiple export fails. Proves: the user +;; gets a toast, and the leave-page warning the export turned on is released +;; even though no progress message will ever arrive. +(t/deftest ^:async failed-multiple-export-shows-a-toast-and-releases-the-leave-warning + (let [store-errors (atom []) + flashed (atom []) + tasks (atom #{}) + store (export-store (test-state) store-errors)] + (await + (mock/with-mocks* + {repo/cmd! (mock/stub (fn [_ _] + (rx/throw (ex-info "exporter failed" + {:type :server-error})))) + dwp/force-persist-files-and-wait (mock/stub (fn [_ _] (rx/empty))) + ws/get-rcv-stream (mock/stub (fn [_] (rx/subject))) + st/ongoing-tasks tasks + errors/flash (fn [& {:keys [hint]}] (swap! flashed conj hint)) + i18n/tr (mock/stub #(str "translated:" %))} + (ptk/emit! store (de/request-multiple-export {:exports [(assoc export :enabled true)]})) + (await (async/wait-for #(seq @flashed) "the failure is shown")) + (await (async/wait-for #(not (contains? @tasks :export)) + "the leave-page warning is released")) + (t/is (= ["translated:errors.export-failed"] @flashed)) + (t/is (empty? @store-errors) "nothing reaches the global error handler"))))) + +;; Scenario: an export starts while its file has a change not saved yet. +;; Proves: the user is warned that the export may miss it, and the export +;; runs anyway. +(t/deftest ^:async export-warns-when-the-file-has-unsaved-changes + (let [file-id (uuid/next) + downloaded (atom false) + store (export-store (unsaved-state file-id) (atom []))] + (await + (mock/with-mocks* + {repo/cmd! (mock/stub (fn [_ _] + (rx/of {:filename "export.png" + :mtype "image/png" + :uri "blob:export"}))) + dwp/force-persist-files-and-wait (mock/stub (fn [_ _] (rx/empty))) + dom/trigger-download-uri (mock/stub (fn [& _] (reset! downloaded true))) + i18n/tr (mock/stub #(str "translated:" %))} + (ptk/emit! store (de/request-simple-export {:export (assoc export :file-id file-id)})) + (await (async/wait-for #(true? @downloaded) "the export runs anyway")) + (t/is (= "translated:workspace.export.unsaved-changes" + (get-in @store [:notification :content]))) + (t/is (= :warning (get-in @store [:notification :level]))))))) + +;; Scenario: an export starts with every change of its file saved. Proves: +;; no warning is shown. +(t/deftest ^:async export-shows-no-warning-when-the-file-is-saved + (let [downloaded (atom false) + store (export-store (test-state) (atom []))] + (await + (mock/with-mocks* + {repo/cmd! (mock/stub (fn [_ _] + (rx/of {:filename "export.png" + :mtype "image/png" + :uri "blob:export"}))) + dwp/force-persist-files-and-wait (mock/stub (fn [_ _] (rx/empty))) + dom/trigger-download-uri (mock/stub (fn [& _] (reset! downloaded true)))} + (ptk/emit! store (de/request-simple-export {:export (assoc export :file-id (uuid/next))})) + (await (async/wait-for #(true? @downloaded) "the export runs")) + (await (async/settle)) + (t/is (nil? (:notification @store))))))) diff --git a/frontend/test/frontend_tests/data/persistence_test.cljs b/frontend/test/frontend_tests/data/persistence_test.cljs index 80ba7608bd..32150ad8dd 100644 --- a/frontend/test/frontend_tests/data/persistence_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_test.cljs @@ -26,6 +26,7 @@ [app.main.data.render-wasm :as drw] [app.main.data.workspace :as dw] [app.main.errors :as errors] + [app.main.refs :as refs] [app.main.repo :as rp] [app.main.store :as st] [app.util.i18n :as i18n] @@ -33,6 +34,7 @@ [cljs.test :as t :include-macros true] [frontend-tests.helpers.async :as async] [frontend-tests.helpers.mock :as mock] + [okulary.core :as l] [potok.v2.core :as ptk])) (t/deftest reopening-restores-only-the-retained-edits-of-the-loaded-file @@ -1003,6 +1005,71 @@ "the release turns the guard off")) (t/is (= :saved (get-in @store [:persistence :status]))))))) +(defn- wait-files-saved + "Emits into `store` an event that runs `force-persist-files-and-wait` for + `file-ids` and then flips `done`." + [store file-ids timeout-ms done] + (ptk/emit! store + (ptk/reify ::wait-files-saved + ptk/WatchEvent + (watch [_ _ _] + (rx/concat + (dps/force-persist-files-and-wait file-ids timeout-ms) + (rx/of (ptk/reify ::files-saved + ptk/EffectEvent + (effect [_ _ _] (reset! done true))))))))) + +;; Scenario: an edit waits in the buffer when an export asks for its file +;; to be saved. Proves: the wait sends the edit and only ends once the save +;; lands. +(t/deftest ^:async waiting-for-a-file-sends-and-waits-for-its-edits + (let [done (atom false)] + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (await + (mock/with-mocks* + {refs/persistence (l/derived :persistence store)} + (ptk/emit! store (local-commit file-id)) + (wait-files-saved store #{file-id} 60000 done) + (await (async/wait-for #(= 1 (count @requests)) "the edit is sent at once")) + (await (async/settle)) + (t/is (false? @done) "the wait holds while the save is in flight") + (rx/push! response {:revn 1}) + (await (async/wait-for #(true? @done) "the landed save ends the wait")) + (t/is (not (dps/unsaved-changes? @store #{file-id})))))))))) + +;; Scenario: a save of one file hangs while another file is exported. +;; Proves: the wait only covers the exported files. +(t/deftest ^:async waiting-for-a-file-ignores-other-files + (let [done (atom false)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {refs/persistence (l/derived :persistence store)} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the other file's save hangs")) + (wait-files-saved store #{(uuid/next)} 60000 done) + (await (async/wait-for #(true? @done) "the wait ends at once"))))))))) + +;; Scenario: the file's save hangs past the wait. Proves: the wait gives up +;; without raising, and the file still reports unsaved changes. +(t/deftest ^:async waiting-for-a-file-gives-up-after-the-timeout + (let [done (atom false)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {refs/persistence (l/derived :persistence store)} + (ptk/emit! store (local-commit file-id)) + (wait-files-saved store #{file-id} 1 done) + (await (async/wait-for #(true? @done) "the wait times out")) + (t/is (= 1 (count @requests))) + (t/is (dps/unsaved-changes? @store #{file-id}))))))))) + (defn- save-failed-events "Starts collecting the `save-failed` audit events that `store` processes." [store] diff --git a/frontend/translations/en.po b/frontend/translations/en.po index e08161f63b..4269991302 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1589,6 +1589,10 @@ msgstr "The email «%s» has many permanent bounce reports." msgid "errors.email-spam-or-permanent-bounces" msgstr "The email «%s» has been reported as spam or permanently bounce." +#: src/app/main/data/exports/assets.cljs +msgid "errors.export-failed" +msgstr "The export failed. Try again in a moment." + #: src/app/main/errors.cljs:391 msgid "errors.feature-mismatch" msgstr "" @@ -6919,6 +6923,10 @@ msgstr "Remove guide" msgid "workspace.debug.title" msgstr "Debugging tools" +#: src/app/main/data/exports/assets.cljs +msgid "workspace.export.unsaved-changes" +msgstr "Some of your latest changes are not saved yet, so this export may not include them." + #: src/app/main/ui/workspace/sidebar/layers.cljs:841 msgid "workspace.focus.focus-mode" msgstr "Focus mode" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index 3807fd81d0..2c807ed06f 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -1573,6 +1573,10 @@ msgstr "El correo electrónico «%s» tiene varios reportes de rebote permanente msgid "errors.email-spam-or-permanent-bounces" msgstr "El email «%s» tiene reportes de spam o de rebote permanente." +#: src/app/main/data/exports/assets.cljs +msgid "errors.export-failed" +msgstr "La exportación ha fallado. Inténtalo de nuevo en un momento." + #: src/app/main/errors.cljs:391 msgid "errors.feature-mismatch" msgstr "" @@ -6757,6 +6761,10 @@ msgstr "Duplicar fila" msgid "workspace.debug.title" msgstr "Herramientas de depuración" +#: src/app/main/data/exports/assets.cljs +msgid "workspace.export.unsaved-changes" +msgstr "Algunos de tus últimos cambios aún no se han guardado, así que puede que esta exportación no los incluya." + #: src/app/main/ui/workspace/sidebar/layers.cljs:841 msgid "workspace.focus.focus-mode" msgstr "Modo foco"