From a091c9c9106039499140cd1c5268a1795ad47763 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 21 Aug 2024 11:10:16 +0200 Subject: [PATCH 1/4] :recycle: Refactor how UI error reporting is handled --- frontend/package.json | 1 + frontend/src/app/main/errors.cljs | 21 ++--- frontend/src/app/main/ui.cljs | 18 ++-- frontend/src/app/main/ui/error_boundary.cljs | 47 +++++++++++ frontend/src/app/main/ui/static.cljs | 88 ++++++++++++-------- frontend/yarn.lock | 21 +++++ 6 files changed, 137 insertions(+), 59 deletions(-) create mode 100644 frontend/src/app/main/ui/error_boundary.cljs diff --git a/frontend/package.json b/frontend/package.json index 7e816f8c0..67bb13612 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -103,6 +103,7 @@ "randomcolor": "^0.6.2", "react": "18.3.1", "react-dom": "18.3.1", + "react-error-boundary": "^4.0.13", "react-virtualized": "^9.22.5", "rxjs": "8.0.0-alpha.14", "sax": "^1.4.1", diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 38fd01c22..59891188b 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -56,6 +56,14 @@ (print-explain! cause) (print-trace! cause)))) +(defn exception->error-data + [cause] + (let [data (ex-data cause)] + (-> data + (assoc :hint (or (:hint data) (ex-message cause))) + (assoc ::instance cause) + (assoc ::trace (.-stack cause))))) + (defn print-error! [cause] (cond @@ -66,22 +74,14 @@ (print-cause! (ex-message cause) (ex-data cause)) :else - (let [trace (.-stack cause)] - (print-cause! (ex-message cause) - {:hint (ex-message cause) - ::trace trace - ::instance cause})))) + (print-cause! (ex-message cause) (exception->error-data cause)))) (defn on-error "A general purpose error handler." [error] (if (map? error) (ptk/handle-error error) - (let [data (ex-data error) - data (-> data - (assoc :hint (or (:hint data) (ex-message error))) - (assoc ::instance error) - (assoc ::trace (.-stack error)))] + (let [data (exception->error-data error)] (ptk/handle-error data)))) ;; Set the main potok error handler @@ -285,6 +285,7 @@ (let [message (ex-message cause)] (or (= message "Possible side-effect in debug-evaluate") (= message "Unexpected end of input") + (str/starts-with? message "invalid props on component") (str/starts-with? message "Unexpected token ")))) (on-unhandled-error [event] diff --git a/frontend/src/app/main/ui.cljs b/frontend/src/app/main/ui.cljs index 57e1b7372..50cd947e6 100644 --- a/frontend/src/app/main/ui.cljs +++ b/frontend/src/app/main/ui.cljs @@ -8,9 +8,9 @@ (:require [app.config :as cf] [app.main.refs :as refs] - [app.main.store :as st] [app.main.ui.context :as ctx] [app.main.ui.debug.icons-preview :refer [icons-preview]] + [app.main.ui.error-boundary :refer [error-boundary*]] [app.main.ui.frame-preview :as frame-preview] [app.main.ui.icons :as i] [app.main.ui.notifications :as notifications] @@ -21,7 +21,6 @@ [app.main.ui.static :as static] [app.util.dom :as dom] [app.util.i18n :refer [tr]] - [app.util.router :as rt] [rumext.v2 :as mf])) (def auth-page @@ -42,15 +41,8 @@ (def workspace-page (mf/lazy-component app.main.ui.workspace/workspace)) -(mf/defc on-main-error - [{:keys [error] :as props}] - (mf/with-effect - (st/emit! (rt/assign-exception error))) - [:span "Internal application error"]) - (mf/defc main-page - {::mf/wrap [#(mf/catch % {:fallback on-main-error})] - ::mf/props :obj} + {::mf/props :obj} [{:keys [route profile]}] (let [{:keys [data params]} route] [:& (mf/provider ctx/current-route) {:value route} @@ -137,7 +129,7 @@ {:keys [file-id]} path-params] [:? {} (if (:token query-params) - [:> static/error-container {} + [:> static/error-container* {} [:div.image i/detach] [:div.main-message (tr "viewer.breaking-change.message")] [:div.desc-message (tr "viewer.breaking-change.description")]] @@ -186,8 +178,8 @@ [:& (mf/provider ctx/current-route) {:value route} [:& (mf/provider ctx/current-profile) {:value profile} (if edata - [:& static/exception-page {:data edata :route route}] - [:* + [:> static/exception-page* {:data edata :route route}] + [:> error-boundary* {:fallback static/internal-error*} [:& notifications/current-notification] (when route [:& main-page {:route route :profile profile}])])]])) diff --git a/frontend/src/app/main/ui/error_boundary.cljs b/frontend/src/app/main/ui/error_boundary.cljs new file mode 100644 index 000000000..f34f00941 --- /dev/null +++ b/frontend/src/app/main/ui/error_boundary.cljs @@ -0,0 +1,47 @@ +;; 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 app.main.ui.error-boundary + "React error boundary components" + (:require + ["react-error-boundary" :as reb] + [app.main.errors :as errors] + [app.main.refs :as refs] + [goog.functions :as gfn] + [rumext.v2 :as mf])) + +(mf/defc error-boundary* + {::mf/props :obj} + [{:keys [fallback children]}] + (let [fallback-wrapper + (mf/with-memo [fallback] + (mf/fnc fallback-wrapper* + {::mf/props :obj} + [{:keys [error reset-error-boundary]}] + (let [route (mf/deref refs/route) + data (errors/exception->error-data error)] + [:> fallback {:data data + :route route + :on-reset reset-error-boundary}]))) + + on-error + (mf/with-memo [] + ;; NOTE: The debounce is necessary just for simplicity, + ;; becuase for some reasons the error is reported twice in a + ;; very small amount of time, so we debounce for 100ms for + ;; avoid duplicate and redundant reports + (gfn/debounce (fn [error info] + (js/console.error + "Component trace: \n" + (unchecked-get info "componentStack") + "\n" + error)) + 100))] + + [:> reb/ErrorBoundary + {:FallbackComponent fallback-wrapper + :onError on-error} + children])) diff --git a/frontend/src/app/main/ui/static.cljs b/frontend/src/app/main/ui/static.cljs index dea1570f6..06333087c 100644 --- a/frontend/src/app/main/ui/static.cljs +++ b/frontend/src/app/main/ui/static.cljs @@ -29,8 +29,8 @@ [potok.v2.core :as ptk] [rumext.v2 :as mf])) -(mf/defc error-container - {::mf/wrap-props false} +(mf/defc error-container* + {::mf/props :obj} [{:keys [children]}] (let [profile-id (:profile-id @st/state)] [:section {:class (stl/css :exception-layout)} @@ -57,12 +57,10 @@ (mf/defc invalid-token [] - [:> error-container {} + [:> error-container* {} [:div {:class (stl/css :main-message)} (tr "errors.invite-invalid")] [:div {:class (stl/css :desc-message)} (tr "errors.invite-invalid.info")]]) - - (mf/defc login-dialog {::mf/props :obj} [{:keys [show-dialog]}] @@ -247,37 +245,49 @@ [:& request-dialog {:title (tr "not-found.no-permission.project") :button-text (tr "not-found.no-permission.go-dashboard")}] (and (some? file-id) (:already-requested requested)) - [:& request-dialog {:title (tr "not-found.no-permission.already-requested.file") :content [(tr "not-found.no-permission.already-requested.or-others.file")] :button-text (tr "not-found.no-permission.go-dashboard")}] + [:& request-dialog {:title (tr "not-found.no-permission.already-requested.file") + :content [(tr "not-found.no-permission.already-requested.or-others.file")] + :button-text (tr "not-found.no-permission.go-dashboard")}] (:already-requested requested) - [:& request-dialog {:title (tr "not-found.no-permission.already-requested.project") :content [(tr "not-found.no-permission.already-requested.or-others.project")] :button-text (tr "not-found.no-permission.go-dashboard")}] + [:& request-dialog {:title (tr "not-found.no-permission.already-requested.project") + :content [(tr "not-found.no-permission.already-requested.or-others.project")] + :button-text (tr "not-found.no-permission.go-dashboard")}] (:sent requested) - [:& request-dialog {:title (tr "not-found.no-permission.done.success") :content [(tr "not-found.no-permission.done.remember")] :button-text (tr "not-found.no-permission.go-dashboard")}] + [:& request-dialog {:title (tr "not-found.no-permission.done.success") + :content [(tr "not-found.no-permission.done.remember")] + :button-text (tr "not-found.no-permission.go-dashboard")}] (some? file-id) - [:& request-dialog {:title (tr "not-found.no-permission.file") :content [(tr "not-found.no-permission.you-can-ask.file") (tr "not-found.no-permission.if-approves")] :button-text (tr "not-found.no-permission.ask") :on-button-click on-request-access :cancel-text (tr "not-found.no-permission.go-dashboard")}] + [:& request-dialog {:title (tr "not-found.no-permission.file") + :content [(tr "not-found.no-permission.you-can-ask.file") + (tr "not-found.no-permission.if-approves")] + :button-text (tr "not-found.no-permission.ask") + :on-button-click on-request-access + :cancel-text (tr "not-found.no-permission.go-dashboard")}] (some? team-id) - [:& request-dialog {:title (tr "not-found.no-permission.project") :content [(tr "not-found.no-permission.you-can-ask.project") (tr "not-found.no-permission.if-approves")] :button-text (tr "not-found.no-permission.ask") :on-button-click on-request-access :cancel-text (tr "not-found.no-permission.go-dashboard")}]))])) + [:& request-dialog {:title (tr "not-found.no-permission.project") + :content [(tr "not-found.no-permission.you-can-ask.project") + (tr "not-found.no-permission.if-approves")] + :button-text (tr "not-found.no-permission.ask") + :on-button-click on-request-access + :cancel-text (tr "not-found.no-permission.go-dashboard")}]))])) - - -(mf/defc not-found +(mf/defc not-found* [] - [:> error-container {} + [:> error-container* {} [:div {:class (stl/css :main-message)} (tr "labels.not-found.main-message")] [:div {:class (stl/css :desc-message)} (tr "not-found.desc-message.error")] [:div {:class (stl/css :desc-message)} (tr "not-found.desc-message.doesnt-exist")]]) - - -(mf/defc bad-gateway +(mf/defc bad-gateway* [] (let [handle-retry (mf/use-fn (fn [] (st/emit! (rt/assign-exception nil))))] - [:> error-container {} + [:> error-container* {} [:div {:class (stl/css :main-message)} (tr "labels.bad-gateway.main-message")] [:div {:class (stl/css :desc-message)} (tr "labels.bad-gateway.desc-message")] [:div {:class (stl/css :sign-info)} @@ -286,13 +296,12 @@ (mf/defc service-unavailable [] (let [on-click (mf/use-fn #(st/emit! (rt/assign-exception nil)))] - [:> error-container {} + [:> error-container* {} [:div {:class (stl/css :main-message)} (tr "labels.service-unavailable.main-message")] [:div {:class (stl/css :desc-message)} (tr "labels.service-unavailable.desc-message")] [:div {:class (stl/css :sign-info)} [:button {:on-click on-click} (tr "labels.retry")]]])) - (defn generate-report [data] (try @@ -336,17 +345,16 @@ (println))] (wapi/create-blob content "text/plain")) - (catch :default err - (.error js/console err) + (catch :default cause + (.error js/console "error on generating report.txt" cause) nil))) - -(mf/defc internal-error +(mf/defc internal-error* {::mf/props :obj} - [{:keys [data]}] - (let [on-click (mf/use-fn #(st/emit! (rt/assign-exception nil))) - report-uri (mf/use-ref nil) + [{:keys [data on-reset] :as props}] + (let [report-uri (mf/use-ref nil) report (mf/use-memo (mf/deps data) #(generate-report data)) + on-reset (or on-reset #(st/emit! (rt/assign-exception nil))) on-download (mf/use-fn @@ -357,22 +365,24 @@ (mf/with-effect [report] (when (some? report) + (let [uri (wapi/create-uri report)] (mf/set-ref-val! report-uri uri) (fn [] (wapi/revoke-uri uri))))) - [:> error-container {} + [:> error-container* {} [:div {:class (stl/css :main-message)} (tr "labels.internal-error.main-message")] [:div {:class (stl/css :desc-message)} (tr "labels.internal-error.desc-message")] (when (some? report) [:a {:on-click on-download} "Download report.txt"]) [:div {:class (stl/css :sign-info)} - [:button {:on-click on-click} (tr "labels.retry")]]])) + [:button {:on-click on-reset} (tr "labels.retry")]]])) -(mf/defc exception-page +(mf/defc exception-page* {::mf/props :obj} [{:keys [data route] :as props}] + (let [file-info (mf/use-state {:pending true}) team-info (mf/use-state {:pending true}) type (:type data) @@ -409,18 +419,24 @@ (case (:type data) :not-found (if request-access? - [:& request-access {:file-id (:file-id @file-info) :team-id (:team-id @team-info) :is-default (:is-default @team-info) :workspace? workspace?}] - [:& not-found]) + [:& request-access {:file-id (:file-id @file-info) + :team-id (:team-id @team-info) + :is-default (:is-default @team-info) + :workspace? workspace?}] + [:> not-found* {}]) :authentication (if request-access? - [:& request-access {:file-id (:file-id @file-info) :team-id (:team-id @team-info) :is-default (:is-default @team-info) :workspace? workspace?}] - [:& not-found]) + [:& request-access {:file-id (:file-id @file-info) + :team-id (:team-id @team-info) + :is-default (:is-default @team-info) + :workspace? workspace?}] + [:> not-found* {}]) :bad-gateway - [:& bad-gateway] + [:> bad-gateway* props] :service-unavailable [:& service-unavailable] - [:> internal-error props]))) + [:> internal-error* props]))) diff --git a/frontend/yarn.lock b/frontend/yarn.lock index 2bf90d9c7..24131d15b 100644 --- a/frontend/yarn.lock +++ b/frontend/yarn.lock @@ -1738,6 +1738,15 @@ __metadata: languageName: node linkType: hard +"@babel/runtime@npm:^7.12.5": + version: 7.25.0 + resolution: "@babel/runtime@npm:7.25.0" + dependencies: + regenerator-runtime: "npm:^0.14.0" + checksum: 10c0/bd3faf246170826cef2071a94d7b47b49d532351360ecd17722d03f6713fd93a3eb3dbd9518faa778d5e8ccad7392a7a604e56bd37aaad3f3aa68d619ccd983d + languageName: node + linkType: hard + "@babel/runtime@npm:^7.17.8, @babel/runtime@npm:^7.21.0, @babel/runtime@npm:^7.7.2, @babel/runtime@npm:^7.8.4, @babel/runtime@npm:^7.8.7": version: 7.24.5 resolution: "@babel/runtime@npm:7.24.5" @@ -6657,6 +6666,7 @@ __metadata: randomcolor: "npm:^0.6.2" react: "npm:18.3.1" react-dom: "npm:18.3.1" + react-error-boundary: "npm:^4.0.13" react-virtualized: "npm:^9.22.5" rimraf: "npm:^5.0.7" rxjs: "npm:8.0.0-alpha.14" @@ -10690,6 +10700,17 @@ __metadata: languageName: node linkType: hard +"react-error-boundary@npm:^4.0.13": + version: 4.0.13 + resolution: "react-error-boundary@npm:4.0.13" + dependencies: + "@babel/runtime": "npm:^7.12.5" + peerDependencies: + react: ">=16.13.1" + checksum: 10c0/6f3e0e4d7669f680ccf49c08c9571519c6e31f04dcfc30a765a7136c7e6fbbbe93423dd5a9fce12107f8166e54133e9dd5c2079a00c7a38201ac811f7a28b8e7 + languageName: node + linkType: hard + "react-is@npm:18.1.0": version: 18.1.0 resolution: "react-is@npm:18.1.0" From 868af29d148f6b111a3474b8cba1cd5791898c60 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 23 Aug 2024 09:58:05 +0200 Subject: [PATCH 2/4] :lipstick: Add some cosmetic changes to sidebar ns --- .../src/app/main/ui/workspace/sidebar.cljs | 63 +++++++++---------- 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/frontend/src/app/main/ui/workspace/sidebar.cljs b/frontend/src/app/main/ui/workspace/sidebar.cljs index 4cfff5949..5b05aa25a 100644 --- a/frontend/src/app/main/ui/workspace/sidebar.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar.cljs @@ -73,47 +73,40 @@ on-tab-change (mf/use-fn #(st/emit! (dw/go-to-layout (keyword %)))) - tabs (if ^boolean mode-inspect? - #js [#js {:label (tr "workspace.sidebar.layers") - :id "layers" - :content (mf/html [:article {:class (stl/css :layers-tab) - :style #js {"--height" (str size-pages "px")}} + layers-tab + (mf/html + [:article {:class (stl/css :layers-tab) + :style #js {"--height" (str size-pages "px")}} - [:& sitemap {:layout layout - :toggle-pages toggle-pages - :show-pages? @show-pages? - :size size-pages}] + [:& sitemap {:layout layout + :toggle-pages toggle-pages + :show-pages? @show-pages? + :size size-pages}] - (when @show-pages? - [:div {:class (stl/css :resize-area-horiz) - :on-pointer-down on-pointer-down-pages - :on-lost-pointer-capture on-lost-pointer-capture-pages - :on-pointer-move on-pointer-move-pages}]) + (when @show-pages? + [:div {:class (stl/css :resize-area-horiz) + :on-pointer-down on-pointer-down-pages + :on-lost-pointer-capture on-lost-pointer-capture-pages + :on-pointer-move on-pointer-move-pages}]) - [:& layers-toolbox {:size-parent size - :size size-pages}]])}] - #js [#js {:label (tr "workspace.sidebar.layers") - :id "layers" - :content (mf/html [:article {:class (stl/css :layers-tab) - :style #js {"--height" (str size-pages "px")}} + [:& layers-toolbox {:size-parent size + :size size-pages}]]) - [:& sitemap {:layout layout - :toggle-pages toggle-pages - :show-pages? @show-pages? - :size size-pages}] - (when @show-pages? - [:div {:class (stl/css :resize-area-horiz) - :on-pointer-down on-pointer-down-pages - :on-lost-pointer-capture on-lost-pointer-capture-pages - :on-pointer-move on-pointer-move-pages}]) - - [:& layers-toolbox {:size-parent size - :size size-pages}]])} - #js {:label (tr "workspace.toolbar.assets") - :id "assets" - :content (mf/html [:& assets-toolbox {:size (- size 58)}])}])] + assets-tab + (mf/html [:& assets-toolbox {:size (- size 58)}]) + tabs + (if ^boolean mode-inspect? + #js [#js {:label (tr "workspace.sidebar.layers") + :id "layers" + :content layers-tab}] + #js [#js {:label (tr "workspace.sidebar.layers") + :id "layers" + :content layers-tab} + #js {:label (tr "workspace.toolbar.assets") + :id "assets" + :content assets-tab}])] [:& (mf/provider muc/sidebar) {:value :left} [:aside {:ref parent-ref From f16caa2b98f15a362a3068996240b5f1e126d556 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 23 Aug 2024 10:11:33 +0200 Subject: [PATCH 3/4] :lipstick: Add cosmetic changes on sidebar/options ns --- .../main/ui/workspace/sidebar/options.cljs | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/frontend/src/app/main/ui/workspace/sidebar/options.cljs b/frontend/src/app/main/ui/workspace/sidebar/options.cljs index e77c504db..df12fa1c3 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options.cljs @@ -146,25 +146,30 @@ (st/emit! :interrupt (udw/set-workspace-read-only true)) (st/emit! :interrupt (udw/set-workspace-read-only false))))) - design-content (mf/html [:& design-menu {:selected selected - :objects objects - :page-id page-id - :file-id file-id - :selected-shapes selected-shapes - :shapes-with-children shapes-with-children}]) + design-content + (mf/html [:& design-menu {:selected selected + :objects objects + :page-id page-id + :file-id file-id + :selected-shapes selected-shapes + :shapes-with-children shapes-with-children}]) + + inspect-content + (mf/html [:div {:class (stl/css :element-options :inspect-options)} + [:& hrs/right-sidebar {:page-id page-id + :objects objects + :file-id file-id + :frame shape-parent-frame + :shapes selected-shapes + :on-change-section on-change-section + :on-expand on-expand + :from :workspace}]]) + + interactions-content + (mf/html [:div {:class (stl/css :element-options :interaction-options)} + [:& interactions-menu {:shape (first shapes)}]]) - inspect-content (mf/html [:div {:class (stl/css :element-options :inspect-options)} - [:& hrs/right-sidebar {:page-id page-id - :objects objects - :file-id file-id - :frame shape-parent-frame - :shapes selected-shapes - :on-change-section on-change-section - :on-expand on-expand - :from :workspace}]]) - interactions-content (mf/html [:div {:class (stl/css :element-options :interaction-options)} - [:& interactions-menu {:shape (first shapes)}]]) tabs #js [#js {:label (tr "workspace.options.design") :id "design" From 401a28f31788f295e009236bee4979fef2f4b6c9 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 23 Aug 2024 10:13:00 +0200 Subject: [PATCH 4/4] :arrow_up: Update rumext to v2.14 Adds some improvements on compiler --- frontend/deps.edn | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/deps.edn b/frontend/deps.edn index 538d8b156..f13d93fc2 100644 --- a/frontend/deps.edn +++ b/frontend/deps.edn @@ -20,8 +20,8 @@ :git/url "https://github.com/funcool/beicon.git"} funcool/rumext - {:git/tag "v2.13" - :git/sha "dc8e1e5" + {:git/tag "v2.14" + :git/sha "0016623" :git/url "https://github.com/funcool/rumext.git"} instaparse/instaparse {:mvn/version "1.5.0"}