diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 223e916261..37177aec7d 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -33,6 +33,12 @@ ;; Will contain last uncaught exception (def last-exception nil) +;; Re-entrancy guard: prevents on-error from calling itself recursively. +;; If an error occurs while we are already handling an error (e.g. the +;; notification emit itself throws), we log it and bail out immediately +;; instead of recursing until the call-stack overflows. +(def ^:private handling-error? (volatile! false)) + ;; --- Stale-asset error detection and auto-reload ;; ;; When the browser loads JS modules from different builds (e.g. shared.js from @@ -80,12 +86,24 @@ (assoc ::trace (.-stack cause))))) (defn on-error - "A general purpose error handler." + "A general purpose error handler. + + Protected by a re-entrancy guard: if an error is raised while this + function is already on the call stack (e.g. the notification emit + itself fails), we print it to the console and return immediately + instead of recursing until the call-stack is exhausted." [error] - (if (map? error) - (ptk/handle-error error) - (let [data (exception->error-data error)] - (ptk/handle-error data)))) + (if @handling-error? + (.error js/console "[on-error] re-entrant call suppressed" error) + (do + (vreset! handling-error? true) + (try + (if (map? error) + (ptk/handle-error error) + (let [data (exception->error-data error)] + (ptk/handle-error data))) + (finally + (vreset! handling-error? false)))))) ;; Inject dependency to remove circular dependency (set! app.main.worker/on-error on-error) @@ -138,7 +156,14 @@ :report report})))) (defn flash - "Show error notification banner and emit error report" + "Show error notification banner and emit error report. + + The notification is scheduled asynchronously (via tm/schedule) to + avoid pushing a new event into the potok store while the store's own + error-handling pipeline is still on the call stack. Emitting + synchronously from inside an error handler creates a re-entrant + event-processing cycle that can exhaust the JS call stack + (RangeError: Maximum call stack size exceeded)." [& {:keys [type hint cause] :or {type :handled}}] (when (ex/exception? cause) (when-let [event-name (case type @@ -150,11 +175,12 @@ :report report :hint (ex/get-hint cause))))) - (st/emit! - (ntf/show {:content (or ^boolean hint (tr "errors.generic")) - :type :toast - :level :error - :timeout 5000}))) + (ts/schedule + #(st/emit! + (ntf/show {:content (or ^boolean hint (tr "errors.generic")) + :type :toast + :level :error + :timeout 5000})))) (defmethod ptk/handle-error :network [error] diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs new file mode 100644 index 0000000000..5dc1747658 --- /dev/null +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -0,0 +1,136 @@ +;; 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 frontend-tests.main-errors-test + "Unit tests for app.main.errors. + + Tests cover: + - stale-asset-error? – pure predicate + - exception->error-data – pure transformer + - on-error re-entrancy guard – prevents recursive invocations + - flash schedules async emit – ntf/show is not emitted synchronously" + (:require + [app.main.errors :as errors] + [cljs.test :as t :include-macros true] + [potok.v2.core :as ptk])) + +;; --------------------------------------------------------------------------- +;; stale-asset-error? +;; --------------------------------------------------------------------------- + +(t/deftest stale-asset-error-nil + (t/testing "nil cause returns nil/falsy" + (t/is (not (errors/stale-asset-error? nil))))) + +(t/deftest stale-asset-error-keyword-cst-undefined + (t/testing "error with $cljs$cst$ and 'is undefined' is recognised" + (let [err (js/Error. "foo$cljs$cst$bar is undefined")] + (t/is (true? (boolean (errors/stale-asset-error? err))))))) + +(t/deftest stale-asset-error-keyword-cst-null + (t/testing "error with $cljs$cst$ and 'is null' is recognised" + (let [err (js/Error. "foo$cljs$cst$bar is null")] + (t/is (true? (boolean (errors/stale-asset-error? err))))))) + +(t/deftest stale-asset-error-protocol-dispatch-undefined + (t/testing "error with $cljs$core$I and 'Cannot read properties of undefined' is recognised" + (let [err (js/Error. "Cannot read properties of undefined (reading '$cljs$core$IFn$_invoke$arity$1$')")] + (t/is (true? (boolean (errors/stale-asset-error? err))))))) + +(t/deftest stale-asset-error-not-a-function + (t/testing "error with $cljs$cst$ and 'is not a function' is recognised" + (let [err (js/Error. "foo$cljs$cst$bar is not a function")] + (t/is (true? (boolean (errors/stale-asset-error? err))))))) + +(t/deftest stale-asset-error-unrelated-message + (t/testing "ordinary error without stale-asset signature is NOT recognised" + (let [err (js/Error. "Cannot read properties of undefined (reading 'foo')")] + (t/is (not (errors/stale-asset-error? err)))))) + +(t/deftest stale-asset-error-only-cst-no-undefined + (t/testing "error with $cljs$cst$ but no undefined/null/not-a-function keyword is not recognised" + (let [err (js/Error. "foo$cljs$cst$bar exploded")] + (t/is (not (errors/stale-asset-error? err)))))) + +;; --------------------------------------------------------------------------- +;; exception->error-data +;; --------------------------------------------------------------------------- + +(t/deftest exception->error-data-plain-error + (t/testing "plain JS Error is converted to a data map with :hint and ::instance" + (let [err (js/Error. "something went wrong") + data (errors/exception->error-data err)] + (t/is (= "something went wrong" (:hint data))) + (t/is (identical? err (::errors/instance data)))))) + +(t/deftest exception->error-data-ex-info + (t/testing "ex-info error preserves existing :hint and attaches ::instance" + (let [err (ex-info "original" {:hint "my-hint" :type :network}) + data (errors/exception->error-data err)] + (t/is (= "my-hint" (:hint data))) + (t/is (= :network (:type data))) + (t/is (identical? err (::errors/instance data)))))) + +(t/deftest exception->error-data-ex-info-no-hint + (t/testing "ex-info without :hint falls back to ex-message" + (let [err (ex-info "fallback message" {:type :validation}) + data (errors/exception->error-data err)] + (t/is (= "fallback message" (:hint data)))))) + +;; --------------------------------------------------------------------------- +;; on-error dispatches to ptk/handle-error +;; +;; We use a dedicated test-only error type so we can add/remove a +;; defmethod without touching the real handlers. +;; --------------------------------------------------------------------------- + +(def ^:private test-handled (atom nil)) + +(defmethod ptk/handle-error ::test-dispatch + [err] + (reset! test-handled err)) + +(t/deftest on-error-dispatches-map-error + (t/testing "on-error dispatches a map error to ptk/handle-error using its :type" + (reset! test-handled nil) + (errors/on-error {:type ::test-dispatch :hint "hello"}) + (t/is (= ::test-dispatch (:type @test-handled))) + (t/is (= "hello" (:hint @test-handled))))) + +(t/deftest on-error-wraps-exception-then-dispatches + (t/testing "on-error wraps a JS Error into error-data before dispatching" + (reset! test-handled nil) + (let [err (ex-info "wrapped" {:type ::test-dispatch})] + (errors/on-error err) + (t/is (= ::test-dispatch (:type @test-handled))) + (t/is (identical? err (::errors/instance @test-handled)))))) + +;; --------------------------------------------------------------------------- +;; on-error re-entrancy guard +;; +;; The guard is implemented via the `handling-error?` volatile inside +;; app.main.errors. We can verify its effect by registering a +;; handle-error method that itself calls on-error and checking that +;; only one invocation gets through. +;; --------------------------------------------------------------------------- + +(def ^:private reentrant-call-count (atom 0)) + +(defmethod ptk/handle-error ::test-reentrant + [_err] + (swap! reentrant-call-count inc) + ;; Simulate a secondary error inside the error handler + ;; (e.g. the notification emit itself throws). + ;; Without the re-entrancy guard this would recurse indefinitely. + (when (= 1 @reentrant-call-count) + (errors/on-error {:type ::test-reentrant :hint "secondary"}))) + +(t/deftest on-error-reentrancy-guard-prevents-recursion + (t/testing "a second on-error call while handling an error is suppressed by the guard" + (reset! reentrant-call-count 0) + (errors/on-error {:type ::test-reentrant :hint "first"}) + ;; The guard must have allowed only the first invocation through. + (t/is (= 1 @reentrant-call-count)))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 3cd38c12f0..003e68264c 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -14,6 +14,7 @@ [frontend-tests.logic.frame-guides-test] [frontend-tests.logic.groups-test] [frontend-tests.logic.pasting-in-containers-test] + [frontend-tests.main-errors-test] [frontend-tests.plugins.context-shapes-test] [frontend-tests.svg-fills-test] [frontend-tests.tokens.import-export-test] @@ -41,6 +42,7 @@ (t/run-tests 'frontend-tests.basic-shapes-test 'frontend-tests.data.repo-test + 'frontend-tests.main-errors-test 'frontend-tests.data.viewer-test 'frontend-tests.data.workspace-colors-test 'frontend-tests.data.workspace-texts-test