🐛 Fix problems when dragging frame with comments (#10460)

* 🐛 Fix undo frame position not undoing comments

* 🐛 Fix problem with hover capturing dragging event

* 🐛 Fix watch updates for comment bubbles

* 🐛 Fix "Maximum update depth" crash on SVG shape transforms

* 🐛 Fix comment geometry problems
This commit is contained in:
Alonso Torres 2026-06-30 13:33:47 +02:00 committed by GitHub
parent 21f646aeee
commit ca81776d04
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 341 additions and 91 deletions

View File

@ -355,11 +355,15 @@
(defn has-point?
[shape point]
(if (or ^boolean (cfh/path-shape? shape)
^boolean (cfh/bool-shape? shape)
^boolean (cfh/circle-shape? shape))
(slow-has-point? shape point)
(fast-has-point? shape point)))
(let [rotation (dm/get-prop shape :rotation)]
;; Rotated shapes don't match their axis-aligned box, so use the polygon test.
(if (or ^boolean (cfh/path-shape? shape)
^boolean (cfh/bool-shape? shape)
^boolean (cfh/circle-shape? shape)
(and (some? rotation)
(not ^boolean (mth/almost-zero? rotation))))
(slow-has-point? shape point)
(fast-has-point? shape point))))
(defn rect-contains-shape?
[rect shape]

View File

@ -8,6 +8,8 @@
(:require
[app.common.features :as ffeat]
[app.common.files.changes :as ch]
[app.common.files.changes-builder :as pcb]
[app.common.geom.point :as gpt]
[app.common.schema :as sm]
[app.common.schema.generators :as sg]
[app.common.schema.test :as smt]
@ -736,6 +738,55 @@
{:num 1000})))
(t/deftest set-comment-thread-position
(let [file-id (uuid/custom 2 2)
page-id (uuid/custom 1 1)
thread-id (uuid/custom 3 1)
frame-id (uuid/custom 4 1)
data (make-file-data file-id page-id)]
(t/testing "stores position and frame-id"
(let [change {:type :set-comment-thread-position
:page-id page-id
:comment-thread-id thread-id
:frame-id frame-id
:position (gpt/point 10 20)}
res (ch/process-changes data [change])]
(t/is (= {:frame-id frame-id :position (gpt/point 10 20)}
(get-in res [:pages-index page-id :comment-thread-positions thread-id])))))
(t/testing "removes the position when frame-id and position are nil"
(let [data (ch/process-changes data [{:type :set-comment-thread-position
:page-id page-id
:comment-thread-id thread-id
:frame-id frame-id
:position (gpt/point 10 20)}])
res (ch/process-changes data [{:type :set-comment-thread-position
:page-id page-id
:comment-thread-id thread-id
:frame-id nil
:position nil}])]
(t/is (nil? (get-in res [:pages-index page-id :comment-thread-positions thread-id])))))
(t/testing "builder round-trips the position through undo and redo"
(let [data (ch/process-changes data [{:type :set-comment-thread-position
:page-id page-id
:comment-thread-id thread-id
:frame-id frame-id
:position (gpt/point 10 20)}])
page (get-in data [:pages-index page-id])
changes (-> (pcb/empty-changes)
(pcb/with-page page)
(pcb/set-comment-thread-position {:id thread-id
:frame-id frame-id
:position (gpt/point 100 200)}))
redone (ch/process-changes data (:redo-changes changes))
undone (ch/process-changes redone (:undo-changes changes))]
(t/is (= (gpt/point 100 200)
(get-in redone [:pages-index page-id :comment-thread-positions thread-id :position])))
(t/is (= (gpt/point 10 20)
(get-in undone [:pages-index page-id :comment-thread-positions thread-id :position])))))))
(t/deftest set-plugin-data-json-encode-decode
(let [schema ch/schema:set-plugin-data-change
encode (sm/encoder schema (sm/json-transformer))

View File

@ -254,3 +254,19 @@
shape {:points points}]
(t/is (true? (gint/slow-has-point? shape (pt 50 25))))
(t/is (false? (gint/slow-has-point? shape (pt 150 25)))))))
(t/deftest has-point-rotated-test
;; Diamond (a square rotated 45º); its axis-aligned x/y/width/height box does
;; not match the rotated polygon.
(let [points [(pt 50 0) (pt 100 50) (pt 50 100) (pt 0 50)]
shape {:x 20 :y 20 :width 60 :height 60 :rotation 45 :points points}]
(t/testing "point inside the polygon but outside the box is contained"
(t/is (true? (gint/has-point? shape (pt 50 5)))))
(t/testing "point inside the box but outside the polygon is not contained"
(t/is (false? (gint/has-point? shape (pt 22 22)))))))
(t/deftest has-point-axis-aligned-test
(let [shape {:x 10 :y 20 :width 100 :height 50 :rotation 0}]
(t/testing "unrotated shape uses the axis-aligned box"
(t/is (true? (gint/has-point? shape (pt 50 40))))
(t/is (false? (gint/has-point? shape (pt 200 40)))))))

View File

@ -438,6 +438,9 @@
(rx/take 1)
(rx/map #(dwcm/navigate-to-comment-id comment-id))))
;; Keep comment thread positions in sync on undo/redo
(rx/of (dwcm/watch-comment-thread-position-changes stoper-s))
(let [local-commits-s
(->> stream
(rx/filter dch/commit?)

View File

@ -8,10 +8,13 @@
(:require
[app.common.data :as d]
[app.common.data.macros :as dm]
[app.common.files.changes-builder :as pcb]
[app.common.geom.matrix :as gmt]
[app.common.geom.point :as gpt]
[app.common.geom.shapes :as gsh]
[app.common.schema :as sm]
[app.common.types.shape-tree :as ctst]
[app.main.data.changes :as dwc]
[app.main.data.comments :as dcmt]
[app.main.data.common :as dcm]
[app.main.data.event :as ev]
@ -126,6 +129,14 @@
ny (- (:y position) nh)]
(update local :vbox assoc :x nx :y ny)))))))
(defn- set-comment-thread
"Stores the comment thread in the workspace state so its bubble re-renders."
[thread]
(ptk/reify ::set-comment-thread
ptk/UpdateEvent
(update [_ state]
(assoc-in state [:comment-threads (:id thread)] thread))))
(defn update-comment-thread-position
([thread [new-x new-y]]
(update-comment-thread-position thread [new-x new-y] nil))
@ -136,35 +147,106 @@
(dcmt/check-comment-thread! thread))
(ptk/reify ::update-comment-thread-position
ptk/WatchEvent
(watch [_ state _]
(watch [it state _]
(let [page (dsh/lookup-page state)
page-id (:id page)
objects (dsh/lookup-page-objects state page-id)
frame-id (if (nil? frame-id)
(ctst/get-frame-id-by-position objects (gpt/point new-x new-y))
(:frame-id thread))
thread (-> thread
(assoc :position (gpt/point new-x new-y))
(assoc :frame-id frame-id))
thread-id (:id thread)]
position (gpt/point new-x new-y)
thread (-> thread
(assoc :position position)
(assoc :frame-id frame-id))
thread-id (:id thread)
;; Record the position as a change so it joins the undo entry
set-position-changes
(-> (pcb/empty-changes it)
(pcb/with-page page)
(pcb/set-comment-thread-position thread))]
(rx/concat
(rx/of (fn [state]
(-> state
(update :comment-threads assoc thread-id thread)
;; Keep the page positions map in sync so subsequent
;; frame moves compute the relative offset from the
;; latest position instead of a stale one.
(dsh/update-page page-id
#(update-in % [:comment-thread-positions thread-id]
(fn [pos]
(-> pos
(assoc :position (:position thread))
(assoc :frame-id (:frame-id thread)))))))))
(->> (rp/cmd! :update-comment-thread-position thread)
;; Update the new position in the rendered thread, and commit the
;; change so the move is part of the undo entry
(rx/of (set-comment-thread thread)
(dwc/commit-changes set-position-changes))
(->> (rp/cmd! :update-comment-thread-position {:id thread-id
:position position
:frame-id frame-id})
(rx/catch #(rx/throw {:type :update-comment-thread-position}))
(rx/ignore))))))))
(def ^:private undo-origins
#{:app.main.data.workspace.undo/undo
:app.main.data.workspace.undo/redo
:app.main.data.workspace.undo/undo-to-index})
(defn- sync-comment-thread-position
"Syncs the rendered thread and the backend for a comment position change."
[{:keys [comment-thread-id position frame-id]}]
(ptk/reify ::sync-comment-thread-position
ptk/UpdateEvent
(update [_ state]
(cond-> state
(and position frame-id)
(update-in [:comment-threads comment-thread-id]
(fn [thread]
(some-> thread (assoc :position position :frame-id frame-id))))))
ptk/WatchEvent
(watch [_ _ _]
(if (and position frame-id)
(->> (rp/cmd! :update-comment-thread-position {:id comment-thread-id
:position position
:frame-id frame-id})
(rx/catch #(rx/throw {:type :update-comment-thread-position}))
(rx/ignore))
(rx/empty)))))
(defn watch-comment-thread-position-changes
"Syncs rendered threads and the backend when an undo/redo changes a comment position."
[stopper]
(ptk/reify ::watch-comment-thread-position-changes
ptk/WatchEvent
(watch [_ _ stream]
(->> stream
(rx/filter dwc/commit?)
(rx/map deref)
(rx/filter #(contains? undo-origins (:origin %)))
(rx/mapcat (fn [commit]
(->> (:redo-changes commit)
(filter #(= :set-comment-thread-position (:type %)))
(rx/from))))
(rx/map sync-comment-thread-position)
(rx/take-until stopper)))))
(defn frame-pin-transform
"Matrix that moves a comment pinned inside `frame` as the frame is transformed,
following its translation and rotation but not its resize scale."
[frame modifiers transform]
(when (and (some? frame) (or (some? modifiers) (some? transform)))
(let [frame' (cond-> frame
(some? modifiers) (gsh/transform-shape modifiers)
(some? transform) (gsh/apply-transform transform))
c (gsh/shape->center frame)
c' (gsh/shape->center frame')
tfi (or (:transform-inverse frame) (gmt/matrix))
tf' (or (:transform frame') (gmt/matrix))
sr (:selrect frame)
sr' (:selrect frame')
d (gpt/point (- (:x sr') (:x sr))
(- (:y sr') (:y sr)))]
(-> (gmt/matrix)
(gmt/translate! c')
(gmt/multiply! tf')
(gmt/translate! (gpt/negate c'))
(gmt/translate! d)
(gmt/translate! c)
(gmt/multiply! tfi)
(gmt/translate! (gpt/negate c))))))
;; Move comment threads that are inside a frame when that frame is moved"
(defn- move-frame-comment-threads
@ -187,25 +269,18 @@
build-move-event
(fn [comment-thread]
(let [frame-id (:frame-id comment-thread)
(let [frame-id (:frame-id comment-thread)
frame (get objects frame-id)
modifiers (get-in object-modifiers [frame-id :modifiers])
transform (get transforms frame-id)
frame'
(cond-> frame
(some? modifiers)
(gsh/transform-shape modifiers)
matrix (frame-pin-transform frame modifiers transform)
(some? transform)
(gsh/apply-transform transform))
moved (gpt/to-vec (gpt/point (:x frame) (:y frame))
(gpt/point (:x frame') (:y frame')))
position (get-in threads-position-map [(:id comment-thread) :position])
new-x (+ (:x position) (:x moved))
new-y (+ (:y position) (:y moved))]
(update-comment-thread-position comment-thread [new-x new-y] (:id frame))))]
position' (cond-> position
(some? matrix)
(gpt/transform matrix))]
(update-comment-thread-position comment-thread [(:x position') (:y position')] frame-id)))]
(->> (:comment-threads state)
(vals)

View File

@ -327,12 +327,21 @@
(dwm/create-modif-tree shape-ids %)
:ignore-constraints (contains? layout :scale-text)))))
(->> resize-events-stream
(rx/mapcat
(let [emit-modifiers
(fn [modifiers]
(let [modif-tree (dwm/create-modif-tree shape-ids modifiers)]
(rx/of (dwm/set-modifiers modif-tree (contains? layout :scale-text))))))
(rx/take-until stopper)))]
(rx/of (dwm/set-modifiers modif-tree (contains? layout :scale-text)))))]
;; Throttle the live preview to limit re-renders; the trailing
;; rx/last applies the exact final frame.
(rx/merge
(->> resize-events-stream
(rx/sample 16)
(rx/mapcat emit-modifiers)
(rx/take-until stopper))
(->> resize-events-stream
(rx/take-until stopper)
(rx/last)
(rx/mapcat emit-modifiers)))))]
(rx/concat
;; This initial stream waits for some pixels to be move before making the resize
@ -523,14 +532,22 @@
(rx/of (finish-transform)))
(rx/concat
(rx/merge
(->> angle-stream
(rx/map
#(dwm/set-rotation-modifiers % shapes group-center))
(rx/take-until stopper)))
(rx/of (dwm/apply-modifiers)
(finish-transform))))))))
(let [emit-modifiers
(fn [angle] (dwm/set-rotation-modifiers angle shapes group-center))]
;; Throttle the live preview to limit re-renders; the trailing
;; rx/last applies the exact final frame.
(rx/concat
(rx/merge
(->> angle-stream
(rx/sample 16)
(rx/map emit-modifiers)
(rx/take-until stopper))
(->> angle-stream
(rx/take-until stopper)
(rx/last)
(rx/map emit-modifiers)))
(rx/of (dwm/apply-modifiers)
(finish-transform)))))))))
(defn increase-rotation
"Rotate shapes a fixed angle, from a keyboard action."
@ -822,6 +839,8 @@
(rx/merge
(->> modifiers-stream
;; Throttle the live preview to limit re-renders.
(rx/sample 16)
(rx/map
(fn [[modifiers snap-ignore-axis]]
(dwm/set-modifiers modifiers false false {:snap-ignore-axis snap-ignore-axis}))))
@ -843,10 +862,13 @@
;; Last event will write the modifiers creating the changes
(->> move-stream
(rx/last)
(rx/with-latest-from modifiers-stream)
(rx/mapcat
(fn [[_ target-frame drop-index drop-cell]]
(fn [[[_ target-frame drop-index drop-cell] [modifiers snap-ignore-axis]]]
(let [undo-id (js/Symbol)]
(rx/of (dwu/start-undo-transaction undo-id)
;; Apply the exact final modifiers; the preview may drop the last frame.
(dwm/set-modifiers modifiers false false {:snap-ignore-axis snap-ignore-axis})
(dwm/apply-modifiers {:undo-transation? false})
(move-shapes-to-frame ids target-frame drop-index drop-cell)
(finish-transform)

View File

@ -1164,6 +1164,9 @@
test-id (str/join "-" (map :seqn (sort-by :seqn thread-group)))
;; Click-through while transforming a shape, so it doesn't capture the drag
dragging? (some? (mf/deref refs/current-transform))
on-click
(mf/use-fn
(mf/deps thread-group position zoom)
@ -1176,7 +1179,8 @@
(dwz/set-zoom position scale-zoom)))))]
[:div {:style {:top (dm/str pos-y "px")
:left (dm/str pos-x "px")}
:left (dm/str pos-x "px")
:pointer-events (when dragging? "none")}
:on-click on-click
:class (stl/css :floating-preview-wrapper :floating-preview-bubble)}
[:> comment-avatar*
@ -1198,6 +1202,9 @@
frame-id (:frame-id thread)
;; Click-through while transforming a shape, so it doesn't capture the drag
dragging? (some? (mf/deref refs/current-transform))
state (mf/use-state
#(do {:is-hover false
:is-grabbing false
@ -1290,7 +1297,8 @@
(on-click thread))))]
[:div {:style {:top (dm/str pos-y "px")
:left (dm/str pos-x "px")}
:left (dm/str pos-x "px")
:pointer-events (when dragging? "none")}
:on-pointer-down on-pointer-down
:on-pointer-up on-pointer-up
:on-pointer-move on-pointer-move

View File

@ -8,9 +8,6 @@
(:require-macros [app.main.style :as stl])
(:require
[app.common.data.macros :as dm]
[app.common.geom.matrix :as gmt]
[app.common.geom.point :as gpt]
[app.common.geom.shapes :as gsh]
[app.main.data.comments :as dcm]
[app.main.data.workspace.comments :as dwcm]
[app.main.refs :as refs]
@ -18,6 +15,47 @@
[app.main.ui.comments :as cmt]
[rumext.v2 :as mf]))
;; Pin transform for the bubble's frame so it follows the frame during a drag,
;; scoped per frame to avoid re-rendering the whole layer each tick.
(defn- use-frame-position-modifier
[frame-id]
(let [modifiers (mf/deref refs/workspace-modifiers)
wasm-mods (mf/deref refs/workspace-wasm-modifiers)
objects (mf/deref refs/workspace-page-objects)]
(dwcm/frame-pin-transform (get objects frame-id)
(get-in modifiers [frame-id :modifiers])
(get wasm-mods frame-id))))
(mf/defc comment-floating-bubble-wrapper*
{::mf/private true}
[{:keys [thread zoom is-open]}]
(let [position-modifier (use-frame-position-modifier (:frame-id thread))]
[:> cmt/comment-floating-bubble*
{:thread thread
:zoom zoom
:position-modifier position-modifier
:is-open is-open}]))
(mf/defc comment-floating-group-wrapper*
{::mf/private true}
[{:keys [thread-group zoom]}]
(let [thread (first thread-group)
position-modifier (use-frame-position-modifier (:frame-id thread))]
[:> cmt/comment-floating-group*
{:thread-group thread-group
:zoom zoom
:position-modifier position-modifier}]))
(mf/defc comment-floating-thread-wrapper*
{::mf/private true}
[{:keys [thread viewport zoom]}]
(let [position-modifier (use-frame-position-modifier (:frame-id thread))]
[:> cmt/comment-floating-thread*
{:thread thread
:viewport viewport
:position-modifier position-modifier
:zoom zoom}]))
(mf/defc comments-layer*
{::mf/wrap [mf/memo]}
[{:keys [vbox vport zoom file-id page-id]}]
@ -34,38 +72,12 @@
threads-map (mf/deref refs/threads)
;; Active transform modifiers (e.g. while dragging a board). We use
;; them to move comment bubbles live alongside their frame, instead of
;; only repositioning them at drop time. The SVG (legacy) renderer keeps
;; them in `:workspace-modifiers`, while the WASM renderer pushes them
;; through the `wasm-modifiers` stream as plain transform matrices.
modifiers (mf/deref refs/workspace-modifiers)
wasm-mods (into {} (mf/deref refs/workspace-wasm-modifiers))
objects (mf/deref refs/workspace-page-objects)
threads
(mf/with-memo [threads-map local profile page-id]
(->> (vals threads-map)
(filter #(= (:page-id %) page-id))
(dcm/apply-filters local profile)))
;; Returns the position translation matrix for a frame that is being
;; transformed, or nil when the frame has no active modifier. The delta
;; matches `move-frame-comment-threads` (frame top-left displacement) so
;; the bubble does not jump when the modifier is committed.
frame-position-modifier
(fn [frame-id]
(when-let [frame (get objects frame-id)]
(let [frame'
(if-let [modifier (get-in modifiers [frame-id :modifiers])]
(gsh/transform-shape frame modifier)
(when-let [transform (get wasm-mods frame-id)]
(gsh/apply-transform frame transform)))]
(when (some? frame')
(let [delta (gpt/to-vec (gpt/point (:x frame) (:y frame))
(gpt/point (:x frame') (:y frame')))]
(gmt/translate-matrix delta))))))
viewport
(assoc vport :offset-x pos-x :offset-y pos-y)
@ -94,23 +106,20 @@
(let [group? (> (count thread-group) 1)
thread (first thread-group)]
(if group?
[:> cmt/comment-floating-group* {:thread-group thread-group
:zoom zoom
:position-modifier (frame-position-modifier (:frame-id thread))
:key (:seqn thread)}]
[:> cmt/comment-floating-bubble* {:thread thread
:zoom zoom
:position-modifier (frame-position-modifier (:frame-id thread))
:is-open (= (:id thread) (:open local))
:key (:seqn thread)}])))
[:> comment-floating-group-wrapper* {:thread-group thread-group
:zoom zoom
:key (:seqn thread)}]
[:> comment-floating-bubble-wrapper* {:thread thread
:zoom zoom
:is-open (= (:id thread) (:open local))
:key (:seqn thread)}])))
(when-let [id (:open local)]
(when-let [thread (get threads-map id)]
(when (seq (dcm/apply-filters local profile [thread]))
[:> cmt/comment-floating-thread*
[:> comment-floating-thread-wrapper*
{:thread thread
:viewport viewport
:position-modifier (frame-position-modifier (:frame-id thread))
:zoom zoom}])))
(when-let [draft (:draft local)]

View File

@ -51,6 +51,7 @@
[frontend-tests.tokens.style-dictionary-test]
[frontend-tests.tokens.token-errors-test]
[frontend-tests.tokens.workspace-tokens-remap-test]
[frontend-tests.ui.comments-position-modifier-test]
[frontend-tests.ui.ds-controls-numeric-input-test]
[frontend-tests.ui.measures-menu-props-test]
[frontend-tests.util-object-test]
@ -121,6 +122,7 @@
'frontend-tests.tokens.style-dictionary-test
'frontend-tests.tokens.token-errors-test
'frontend-tests.tokens.workspace-tokens-remap-test
'frontend-tests.ui.comments-position-modifier-test
'frontend-tests.ui.ds-controls-numeric-input-test
'frontend-tests.ui.measures-menu-props-test
'frontend-tests.render-wasm.process-objects-test

View File

@ -0,0 +1,60 @@
;; 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 Sucursal en España SL
(ns frontend-tests.ui.comments-position-modifier-test
(:require
[app.common.geom.point :as gpt]
[app.common.geom.shapes :as gsh]
[app.common.math :as mth]
[app.common.types.modifiers :as ctm]
[app.common.types.shape :as cts]
[app.common.uuid :as uuid]
[app.main.data.workspace.comments :as dwcm]
[cljs.test :as t :include-macros true]))
(defn- frame
[id]
(cts/setup-shape {:id id :type :frame :name "Board"
:x 100 :y 100 :width 200 :height 150}))
(defn- close-point?
[a b]
(and (mth/close? (:x a) (:x b))
(mth/close? (:y a) (:y b))))
(t/deftest frame-pin-transform-move
(let [f (frame (uuid/next))
mods (ctm/move-modifiers (gpt/point 10 20))
m (dwcm/frame-pin-transform f mods nil)]
(t/testing "the comment follows the frame translation"
(t/is (close-point? (gpt/point 160 170)
(gpt/transform (gpt/point 150 150) m))))))
(t/deftest frame-pin-transform-rotation
(let [f (frame (uuid/next))
center (gsh/shape->center f)
mods (ctm/rotation (ctm/empty) center 90)
m (dwcm/frame-pin-transform f mods nil)
p (gpt/point 150 150)]
(t/testing "the comment rotates around the frame center"
(t/is (close-point? (gpt/transform p (ctm/modifiers->transform mods))
(gpt/transform p m))))))
(t/deftest frame-pin-transform-resize
(let [f (frame (uuid/next))
mods (ctm/resize (ctm/empty) (gpt/point 2 2) (gpt/point 100 100))
m (dwcm/frame-pin-transform f mods nil)
p (gpt/point 150 150)]
(t/testing "the comment keeps its position without scaling"
(t/is (close-point? p (gpt/transform p m))))
(t/testing "the comment is not scaled along with the frame"
(t/is (not (close-point? (gpt/transform p (ctm/modifiers->transform mods))
(gpt/transform p m)))))))
(t/deftest frame-pin-transform-without-transform
(let [f (frame (uuid/next))]
(t/testing "no active transform yields no matrix"
(t/is (nil? (dwcm/frame-pin-transform f nil nil))))))