wip

frontend/src/app/main/fonts.cljs

@@ -139,8 +139,10 @@
 ;; The sprite is heavy (~2000 nodes), so we DON'T keep it in the DOM: the fetched
 ;; markup is cached here as a string (`:svg`) and the node is pre-parsed eagerly
 ;; (`:node`) so attaching is a cheap appendChild. `:ids` are the font ids it
-;; covers (also pre-computed), so the UI can pick sprite vs fallback.
-(defonce preview-sprite (l/atom {:status :idle :ids #{} :svg nil :node nil}))
+;; covers (also pre-computed), so the UI can pick sprite vs fallback. `:refs`
+;; counts concurrent attachers so concurrent selectors share one DOM node
+;; instead of yanking it from under each other.
+(defonce preview-sprite (l/atom {:status :idle :ids #{} :svg nil :node nil :refs 0}))
 
 ;; Id prefix shared with the generator and the UI's `<use href>`; referenced here
 ;; rather than re-declared so the contract stays in one place.
@@ -162,7 +164,7 @@
   []
   ;; :error → the UI shows plain names (no previews, no per-font load storm); a
   ;; later `prefetch-preview-sprite!` call can retry.
-  (reset! preview-sprite {:status :error :ids #{} :svg nil :node nil}))
+  (reset! preview-sprite {:status :error :ids #{} :svg nil :node nil :refs 0}))
 
 (defn- parse-sprite-svg
   "Parse the cached sprite markup as SVG (not HTML, so no innerHTML injection
@@ -219,20 +221,31 @@
   its glyph groups via `<use>`. Returns the node (pass it to
   `detach-preview-sprite!` on close), or nil if not ready. Parsing and id
   collection happen once during `prefetch-preview-sprite!`, so this is just a
-  cheap appendChild."
+  cheap appendChild. Concurrent attachers share the same cached node via a
+  refcount: only the first triggers the DOM append, only the last detach
+  removes it — so two selectors coexisting (right sidebar + in-canvas text
+  popover) don't yank the sprite out from under each other."
   []
-  (let [{:keys [status node]} @preview-sprite]
+  (let [{:keys [status node refs]} @preview-sprite]
     (when (and (globals/browser?) (= :ready status) (some? node))
-      (when-let [body-el (unchecked-get globals/document "body")]
-        (dom/append-child! body-el node))
+      (when (zero? refs)
+        (when-let [body-el (unchecked-get globals/document "body")]
+          (dom/append-child! body-el node)))
+      (swap! preview-sprite update :refs (fnil inc 0))
       node)))
 
 (defn detach-preview-sprite!
   "Remove the sprite node injected by `attach-preview-sprite!` from the DOM. The
   cached markup, parsed node, and `:ids` stay, so reopening re-attaches without
-  a refetch or re-parse."
+  a refetch or re-parse. Refcounted: only the last detach actually removes the
+  node, so concurrent selectors don't pull it out from under each other."
   [node]
-  (dom/remove! node))
+  (let [refs (-> (swap! preview-sprite
+                        (fn [s]
+                          (update s :refs #(max 0 (dec (or % 0))))))
+                 :refs)]
+    (when (zero? refs)
+      (dom/remove! node))))
 
 (defn- add-font-css!
   "Creates a style element and attaches it to the dom."

frontend/src/app/main/ui/workspace/sidebar/options/menus/typography.cljs

@@ -40,6 +40,7 @@
    [app.util.timers :as tm]
    [cuerdas.core :as str]
    [goog.events :as events]
+   [okulary.core :as l]
    [promesa.core :as p]
    [rumext.v2 :as mf]))
 
@@ -102,16 +103,14 @@
   fonts, or the font's own name lazily loaded for custom fonts the sprite doesn't
   cover."
   {::mf/wrap [mf/memo]}
-  [{:keys [font]}]
+  [{:keys [font sprite-ids sprite-ready?]}]
   (let [font-id    (:id font)
-        sprite     (mf/deref fonts/preview-sprite)
-        in-sprite? (contains? (:ids sprite) font-id)
+        in-sprite? (contains? sprite-ids font-id)
 
         ;; Fallback is ONLY for custom fonts: ones the (ready) sprite doesn't
         ;; cover. If the sprite isn't ready (loading/error) we show the plain name
         ;; rather than runtime-loading the whole catalog.
-        fallback?  (and (= :ready (:status sprite))
-                        (not in-sprite?))
+        fallback?  (and sprite-ready? (not in-sprite?))
         loaded?    (use-font-lazy-load font-id fallback?)]
     (if in-sprite?
       ;; `fill: currentColor` (scss) makes the sprite glyph follow the row color.
@@ -126,7 +125,7 @@
 
 (mf/defc font-item*
   {::mf/wrap [mf/memo]}
-  [{:keys [font is-current on-click style]}]
+  [{:keys [font is-current on-click style sprite-ids sprite-ready?]}]
   (let [item-ref (mf/use-ref)
         on-click (mf/use-fn (mf/deps font) #(on-click font))
         ;; FLAG :font-preview — gates the feature markup AND its row styling
@@ -149,7 +148,9 @@
                                   :font-item-preview-on preview?
                                   :selected is-current)}
       (if preview?
-        [:> font-item-preview* {:font font}]
+        [:> font-item-preview* {:font font
+                                :sprite-ids sprite-ids
+                                :sprite-ready? sprite-ready?}]
         [:span {:class (stl/css :font-item-label)} (:name font)])
       (when is-current
         [:> icon* {:icon-id i/tick
@@ -189,7 +190,18 @@
         installed-ids (mf/with-memo [all-fonts]
                         (into #{} (map :id) all-fonts))
 
-        sprite-status (:status (mf/deref fonts/preview-sprite))
+        ;; FLAG :font-preview — narrow the preview-sprite subscription to just
+        ;; `:status`/`:ids`, so unrelated swaps (e.g. a future `:refs` change) and
+        ;; the row reading the whole atom don't re-render every visible row. The
+        ;; lenses are stable across renders via `mf/with-memo []`; we only `deref`
+        ;; them when the flag is on, so the picker doesn't subscribe to the sprite
+        ;; at all when the feature is disabled.
+        preview?      (contains? cf/flags :font-preview)
+        sprite-status (when preview?
+                        (mf/deref (mf/with-memo [] (l/derived :status fonts/preview-sprite))))
+        sprite-ids     (when preview?
+                         (mf/deref (mf/with-memo [] (l/derived :ids fonts/preview-sprite))))
+        sprite-ready?  (= :ready sprite-status)
 
         recent-fonts (mf/deref refs/recent-fonts)
         recent-fonts (mf/with-memo [state recent-fonts installed-ids]
@@ -256,21 +268,19 @@
         #(events/unlistenByKey key)))
 
     ;; FLAG :font-preview — materialize the preview sprite into the DOM only while
-    ;; the picker is open (markup is prefetched on workspace load), removing it on
-    ;; close so its ~2000 nodes aren't kept around idle. The attachment is deferred
-    ;; so the dropdown can paint first with plain names, then the sprite swaps in
-    ;; on the next tick. Remove the flag clause to drop the feature.
-    (mf/with-effect [sprite-status]
-      (when (and (contains? cf/flags :font-preview)
-                 (= :ready sprite-status))
-        (let [node*  (volatile! nil)
-              task   (tm/schedule
-                      (fn []
-                        (vreset! node* (fonts/attach-preview-sprite!))))]
-          (fn []
-            (tm/dispose! task)
-            (when-some [n @node*]
-              (fonts/detach-preview-sprite! n))))))
+    ;; the picker is open. Parsing and id collection happen once in
+    ;; `prefetch-preview-sprite!` (called on workspace mount), so attaching is just
+    ;; a cheap `appendChild` of the cached node — no need to defer it. Removing it
+    ;; on close keeps ~2000 nodes out of the DOM while the picker is closed. The
+    ;; attach is refcounted (see `attach-preview-sprite!`), so concurrent
+    ;; selectors share the node. Remove the flag clause to drop the feature.
+    (when preview?
+      (mf/with-effect [sprite-status]
+        (when (= :ready sprite-status)
+          (let [node (fonts/attach-preview-sprite!)]
+            (fn []
+              (when-some [n node]
+                (fonts/detach-preview-sprite! n)))))))
 
     (mf/with-effect [@selected]
       (when-let [inst (mf/ref-val flist)]
@@ -308,6 +318,8 @@
             [:> font-item* {:key (dm/str "font-" idx)
                             :font font
                             :style {}
+                            :sprite-ids sprite-ids
+                            :sprite-ready? sprite-ready?
                             :on-click on-select-and-close
                             :is-current (= (:id font) (:id effective-selected))}])])]
 
@@ -317,7 +329,7 @@
         (fn [props]
           (let [width  (unchecked-get props "width")
                 height (unchecked-get props "height")
-                render #(row-renderer fonts effective-selected on-select-and-close %)]
+                render #(row-renderer fonts effective-selected on-select-and-close sprite-ids sprite-ready? %)]
             (mf/html
              [:> rvt/List #js {:height height
                                :ref flist
@@ -327,7 +339,7 @@
                                :rowRenderer render}])))]]]]))
 
 (defn row-renderer
-  [fonts selected on-select props]
+  [fonts selected on-select sprite-ids sprite-ready? props]
   (let [index (unchecked-get props "index")
         key   (unchecked-get props "key")
         style (unchecked-get props "style")
@@ -336,6 +348,8 @@
      [:> font-item* {:key key
                      :font font
                      :style style
+                     :sprite-ids sprite-ids
+                     :sprite-ready? sprite-ready?
                      :on-click on-select
                      :is-current (= (:id font) (:id selected))}])))
 

frontend/test/frontend_tests/fonts_test.cljs

@@ -7,6 +7,7 @@
 (ns frontend-tests.fonts-test
   (:require
    [app.main.fonts :as fonts]
+   [app.util.globals :as globals]
    [cljs.test :as t :include-macros true]))
 
 (def sample-font
@@ -124,3 +125,153 @@
           result (fonts/find-closest-variant font "200" nil)]
       (t/is (= "200" (:weight result)))
       (t/is (= "italic" (:style result))))))
+
+;; --- preview sprite attach/detach ----------------------------------------
+;; `prefetch-preview-sprite!` pre-parses the sprite and caches the node under
+;; `:node`; `attach-preview-sprite!` then just appends it and `detach-preview-sprite!`
+;; removes it from the DOM while leaving the cache intact. The typography effect
+;; (in typography.cljs) attaches synchronously on `:ready` and detaches on close;
+;; these tests pin the contract that lets that pattern be re-render- and
+;; reopen-safe: the cached `:node` survives attach/detach cycles in the atom,
+;; the same JS instance is reused on reopen, and attach is a no-op unless
+;; `:ready`+node.
+
+(defn- fake-sprite-state
+  ([node]
+   (fake-sprite-state node 0))
+  ([node refs]
+   {:status :ready :ids #{} :svg "<svg/>" :node node :refs refs}))
+
+(defn- fake-node
+  "A plain JS object that satisfies `dom/remove!` (`.remove`) and can be counted
+  when appended."
+  [append-append-cb]
+  (let [node #js {}]
+    (js/Object.defineProperty node "remove"
+                              #js {:value (fn [] nil) :writable true})
+    (js/Object.defineProperty node "appendChild"
+                              #js {:value (fn [child] (when append-append-cb (append-append-cb)) child)
+                                   :writable true})
+    node))
+
+(t/use-fixtures :each
+  {:before (fn [] (reset! fonts/preview-sprite
+                          {:status :idle :ids #{} :svg nil :node nil :refs 0}))
+   :after  (fn [] (reset! fonts/preview-sprite
+                          {:status :idle :ids #{} :svg nil :node nil :refs 0}))})
+
+(t/deftest attach-preview-sprite-returns-cached-node-test
+  (t/testing "attach returns the same node instance cached by prefetch"
+    (let [node (fake-node nil)]
+      (reset! fonts/preview-sprite (fake-sprite-state node))
+      (with-redefs [globals/browser? (constantly true)
+                    globals/document  #js {:body (fake-node nil)}]
+        (t/is (identical? node (fonts/attach-preview-sprite!)))))))
+
+(t/deftest attach-preview-sprite-nil-unless-ready-test
+  (t/testing "attach is a no-op unless the sprite is :ready with a parsed node"
+    (with-redefs [globals/browser? (constantly true)]
+      (doseq [status [:idle :loading :error]]
+        (reset! fonts/preview-sprite {:status status :ids #{} :svg nil :node nil :refs 0})
+        (t/is (nil? (fonts/attach-preview-sprite!)) (str "status=" status)))
+      ;; :ready but no node (prefetch parsed nothing)
+      (reset! fonts/preview-sprite {:status :ready :ids #{} :svg nil :node nil :refs 0})
+      (t/is (nil? (fonts/attach-preview-sprite!))))))
+
+(t/deftest detach-preview-sprite-keeps-cache-test
+  (t/testing "detach removes the node from the DOM but leaves the cached node
+              in the atom, so reopening reattaches without re-parsing"
+    (let [node (fake-node nil)]
+      (reset! fonts/preview-sprite (fake-sprite-state node))
+      (with-redefs [globals/browser? (constantly true)]
+        (fonts/detach-preview-sprite! node))
+      (t/is (identical? node (:node @fonts/preview-sprite)))
+      (t/is (= :ready (:status @fonts/preview-sprite)))
+      ;; refcount dropped to zero
+      (t/is (zero? (:refs @fonts/preview-sprite))))))
+
+(t/deftest attach-detach-reattach-reuses-same-instance-test
+  (t/testing "reattaching after detach appends the same cached node (no re-parse,
+              no duplicate nodes); mirrors a close/reopen of the font picker"
+    (let [appends (volatile! 0)
+          body    (fake-node #(vswap! appends inc))
+          node    (fake-node nil)]
+      (reset! fonts/preview-sprite (fake-sprite-state node))
+      (with-redefs [globals/browser? (constantly true)
+                    globals/document  #js {:body body}]
+        (let [n1 (fonts/attach-preview-sprite!)
+              _  (fonts/detach-preview-sprite! n1)
+              n2 (fonts/attach-preview-sprite!)]
+          (t/is (identical? n1 n2))
+          (t/is (identical? node n2))
+          ;; Two open cycles → two `appendChild` calls on body. detach doesn't
+          ;; reset the cache, so reopening never re-parses.
+          (t/is (= 2 @appends)))))))
+
+;; --- refcount: concurrent selectors share one DOM node -------------------
+;; Regression test for the bug where two `font-selector*` instances (e.g. right
+;; sidebar + in-canvas text popover) mount at the same time, both read the same
+;; cached `:node`, and the first unmount would `dom/remove!` it out from under
+;; the second. The attach/detach pair is refcounted: the first attach puts the
+;; node in the DOM; subsequent attaches just bump the counter (no second
+;; appendChild); only the last detach removes it.
+
+(t/deftest attach-preview-sprite-refcount-no-double-append-test
+  (t/testing "concurrent attaches only append the DOM node once; the second
+              caller still receives the shared node and the refcount is 2"
+    (let [appends (volatile! 0)
+          body    (fake-node #(vswap! appends inc))
+          node    (fake-node nil)]
+      (reset! fonts/preview-sprite (fake-sprite-state node))
+      (with-redefs [globals/browser? (constantly true)
+                    globals/document #js {:body body}]
+        (let [n1 (fonts/attach-preview-sprite!)
+              n2 (fonts/attach-preview-sprite!)]
+          (t/is (identical? n1 n2))
+          (t/is (identical? node n2))
+          (t/is (= 1 @appends) "appendChild called exactly once for two attaches")
+          (t/is (= 2 (:refs @fonts/preview-sprite))))))))
+
+(t/deftest detach-preview-sprite-refcount-removes-only-on-last-test
+  (t/testing "with two concurrent attachers, detaching once keeps the node in the
+              DOM; only the second (last) detach actually removes it"
+    (let [removes (volatile! 0)
+          node    (fake-node nil)]
+      ;; Track `.remove` invocations on the shared node.
+      (js/Object.defineProperty node "remove"
+                                #js {:value #(vswap! removes inc) :writable true})
+      (reset! fonts/preview-sprite (fake-sprite-state node))
+      (with-redefs [globals/browser? (constantly true)
+                    globals/document #js {:body (fake-node nil)}]
+        (let [_ (fonts/attach-preview-sprite!)
+              _ (fonts/attach-preview-sprite!)]
+          ;; First detach: counter 2→1, node must remain (remove not called)
+          (fonts/detach-preview-sprite! node)
+          (t/is (= 0 @removes) "first detach must not remove the shared node")
+          (t/is (= 1 (:refs @fonts/preview-sprite)))
+          ;; Second detach: counter 1→0, last detach removes the node
+          (fonts/detach-preview-sprite! node)
+          (t/is (= 1 @removes) "last detach removes the node")
+          (t/is (zero? (:refs @fonts/preview-sprite))))))))
+
+(t/deftest detach-preview-sprite-refcount-clamps-at-zero-test
+  (t/testing "spurious detaches (counter already zero) clamp the refcount instead
+              of going negative (defensive against leaked/double detach calls)"
+    (let [node (fake-node nil)]
+      (reset! fonts/preview-sprite (fake-sprite-state node))
+      (with-redefs [globals/browser? (constantly true)]
+        ;; Detach without an attach — counter is 0, must stay 0 (not -1)
+        (fonts/detach-preview-sprite! node)
+        (t/is (zero? (:refs @fonts/preview-sprite)))))))
+
+(t/deftest error-reset-clears-refcount-test
+  (t/testing "`:error` reset wipes the refcount, so a re-prefetch after an error
+              starts from a clean attach state (no stale refs pinning an old
+              detached node)"
+    (reset! fonts/preview-sprite (fake-sprite-state (fake-node nil) 3))
+    (t/is (= 3 (:refs @fonts/preview-sprite)))
+    ;; Call the private reset fn directly (it's the public error path used by
+    ;; prefetch).
+    (#'fonts/reset-preview-sprite-error!)
+    (t/is (zero? (:refs @fonts/preview-sprite)))
+    (t/is (= :error (:status @fonts/preview-sprite)))))

frontend/test/frontend_tests/runner.cljs

@@ -26,6 +26,7 @@
    [frontend-tests.data.workspace-texts-test]
    [frontend-tests.data.workspace-thumbnails-test]
    [frontend-tests.errors-test]
+   [frontend-tests.fonts-test]
    [frontend-tests.helpers-shapes-test]
    [frontend-tests.logic.comp-remove-swap-slots-test]
    [frontend-tests.logic.components-and-tokens]
@@ -118,6 +119,7 @@
    'frontend-tests.data.workspace-texts-test
    'frontend-tests.data.workspace-thumbnails-test
    'frontend-tests.errors-test
+   'frontend-tests.fonts-test
    'frontend-tests.helpers-shapes-test
    'frontend-tests.logic.comp-remove-swap-slots-test
    'frontend-tests.logic.components-and-tokens