🐛 Fix error handling issues (#8962)

* 🚑 Fix RangeError from re-entrant error handling in errors.cljs

Two complementary changes to prevent 'RangeError: Maximum call stack
size exceeded' when an error fires while the potok store error pipeline
is still on the call stack:

1. Re-entrancy guard on on-error: a volatile flag (handling-error?)
   is set true for the duration of each on-error invocation. Any
   nested call (e.g. from a notification emit that itself throws) is
   suppressed with a console.error instead of recursing indefinitely.

2. Async notification in flash: the st/emit!(ntf/show ...) call is
   now wrapped in ts/schedule (setTimeout 0) so the notification event
   is pushed to the store on the next event-loop tick, outside the
   error-handler call stack. This matches the pattern already used by
   the :worker-error, :svg-parser and :comment-error handlers.

* 🐛 Add unit tests for app.main.errors

Test coverage for the error-handling module:

- stale-asset-error?: 6 cases covering keyword-constant and
  protocol-dispatch mismatch signatures, plus negative cases
- exception->error-data: plain JS Error, ex-info with/without :hint
- on-error dispatch: map errors routed via ptk/handle-error, JS
  exceptions wrapped into error-data before dispatch
- Re-entrancy guard: verifies that a second on-error call issued
  from within a handle-error method is suppressed (exactly one
  handler invocation)

---------

Signed-off-by: Andrey Antukh <niwi@niwi.nz>

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]

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))))

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