github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-07-25 15:38:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Throttle nudge stream to cap re-renders under fast key-repeat (#10736)

Browse Source 
Holding an arrow key on a selection with a fast OS key-repeat rate
crashed the workspace with React error #185 (Maximum update depth
exceeded): each OS key-repeat event was converted 1:1 into a
`set-modifiers`/`set-wasm-modifiers` store write inside
`nudge-selected-shapes` with no throttle, starving the renderer.

The sibling mouse-driven resize/rotate/move paths got an `rx/sample`
throttle in PR #10560; the keyboard-nudge path was the only transform
stream left un-throttled. This change applies the same `rx/sample`
throttle to the nudge stream, mirroring the drag-path structure, and
adds a regression test guarding the final committed position
invariant under a burst of 20 `move-selected` events for both the WASM
and legacy (non-WASM) branches.

Closes #10726

AI-assisted-by: glm-5.2
This commit is contained in:
Andrey Antukh 2026-07-23 09:57:28 +02:00 committed by GitHub
parent 702a435569
commit 707cfac375
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 235 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
frontend/src/app/main/data/workspace/transforms.cljs
View File

@ -1023,6 +1023,9 @@
(rx/concat
(rx/merge
(->> modif-stream
;; Sample at a fixed cadence to cap re-renders, mirroring the
;; drag/resize/rotation paths throttled in #10560.
(rx/sample mconst/move-sample-time)
(rx/map #(dwm/set-wasm-modifiers % {:ignore-snap-pixel true})))
(->> modif-stream
@ -1031,17 +1034,28 @@
(rx/of (nudge-selected-shapes direction shift?)))
(rx/of (finish-transform))))
(rx/concat
(rx/merge
(->> move-events
(rx/scan #(gpt/add %1 mov-vec) (gpt/point 0 0))
(rx/map #(dwm/create-modif-tree selected (ctm/move-modifiers %)))
(rx/map #(dwm/set-modifiers % false true))
(rx/take-until stopper))
(rx/of (nudge-selected-shapes direction shift?)))
(let [modif-stream
(->> move-events
(rx/scan #(gpt/add %1 mov-vec) (gpt/point 0 0))
(rx/map #(dwm/create-modif-tree selected (ctm/move-modifiers %)))
(rx/take-until stopper))]
(rx/concat
(rx/merge
(->> modif-stream
;; Sample at a fixed cadence to cap re-renders, mirroring the
;; drag/resize/rotation paths throttled in #10560.
(rx/sample mconst/move-sample-time)
(rx/map #(dwm/set-modifiers % false true)))
;; Un-sampled final write ensures the modifiers atom holds the
;; exact cumulative position before `apply-modifiers` commits,
;; even if `sample` drops the tail value on completion.
(->> modif-stream
(rx/last)
(rx/map #(dwm/set-modifiers % false true)))
(rx/of (nudge-selected-shapes direction shift?)))
(rx/of (dwm/apply-modifiers)
(finish-transform)))))
(rx/of (dwm/apply-modifiers)
(finish-transform))))))
(rx/empty))))))
(defn move-selected

25
frontend/test/frontend_tests/helpers/state.cljs
View File

@ -33,17 +33,20 @@
(pprint (sm/humanize-explain data))))
(defn setup-store
[file]
(let [state (-> initial-state
(assoc :current-file-id (:id file)
:current-page-id (cthf/current-page-id file)
:permissions {:can-edit true}
:files {(:id file) file}))
store (ptk/store {:state state :on-error on-error})]
;; Unit tests skip team/workspace bootstrap; mirror team init so
;; :features is populated the same way as features/initialize does in app.
(ptk/emit! store (features/initialize #{}))
store))
([file] (setup-store file nil))
([file {:keys [renderer] :as _opts}]
(let [state (-> initial-state
(assoc :current-file-id (:id file)
:current-page-id (cthf/current-page-id file)
:permissions {:can-edit true}
:files {(:id file) file})
(cond-> (some? renderer)
(assoc-in [:profile :props :renderer] renderer)))
store (ptk/store {:state state :on-error on-error})]
;; Unit tests skip team/workspace bootstrap; mirror team init so
;; :features is populated the same way as features/initialize does in app.
(ptk/emit! store (features/initialize #{}))
store)))
(defn run-store
([store done events completed-cb]

23
frontend/test/frontend_tests/helpers/wasm.cljs
View File

@ -86,6 +86,20 @@
(track! :set-shape-grow-type)
nil)
(defn- mock-set-modifiers-start
[]
(track! :set-modifiers-start)
nil)
(defn- mock-set-modifiers-end
[]
(track! :set-modifiers-end)
nil)
(defn- mock-get-selection-rect
([_ids] (track! :get-selection-rect) nil)
([_ids _] (track! :get-selection-rect) nil))
(defn- mock-initialized?
[]
(track! :initialized?)
@ -181,6 +195,9 @@
:set-shape-text-content wasm.api/set-shape-text-content
:set-shape-text-images wasm.api/set-shape-text-images
:get-text-dimensions wasm.api/get-text-dimensions
:set-modifiers-start wasm.api/set-modifiers-start
:set-modifiers-end wasm.api/set-modifiers-end
:get-selection-rect wasm.api/get-selection-rect
:font-stored? wasm.fonts/font-stored?
:make-font-data wasm.fonts/make-font-data
:get-content-fonts wasm.fonts/get-content-fonts})
@ -197,6 +214,9 @@
(set! wasm.api/set-shape-text-content mock-set-shape-text-content)
(set! wasm.api/set-shape-text-images mock-set-shape-text-images)
(set! wasm.api/get-text-dimensions mock-get-text-dimensions)
(set! wasm.api/set-modifiers-start mock-set-modifiers-start)
(set! wasm.api/set-modifiers-end mock-set-modifiers-end)
(set! wasm.api/get-selection-rect mock-get-selection-rect)
(set! wasm.fonts/font-stored? mock-font-stored?)
(set! wasm.fonts/make-font-data mock-make-font-data)
(set! wasm.fonts/get-content-fonts mock-get-content-fonts))
@ -217,6 +237,9 @@
(set! wasm.api/set-shape-text-content (:set-shape-text-content orig))
(set! wasm.api/set-shape-text-images (:set-shape-text-images orig))
(set! wasm.api/get-text-dimensions (:get-text-dimensions orig))
(set! wasm.api/set-modifiers-start (:set-modifiers-start orig))
(set! wasm.api/set-modifiers-end (:set-modifiers-end orig))
(set! wasm.api/get-selection-rect (:get-selection-rect orig))
(set! wasm.fonts/font-stored? (:font-stored? orig))
(set! wasm.fonts/make-font-data (:make-font-data orig))
(set! wasm.fonts/get-content-fonts (:get-content-fonts orig)))

172
frontend/test/frontend_tests/logic/nudge_selected_shapes_test.cljs Normal file
View File

@ -0,0 +1,172 @@
;; 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.logic.nudge-selected-shapes-test
"Regression tests for the keyboard-nudge transform stream.
Holding an arrow key on a selection drives `nudge-selected-shapes`
through OS key-repeat. Before the throttle introduced for issue #10726,
every key-repeat was mapped 1:1 into a `set-modifiers`/`set-wasm-modifiers`
store write, which could starve the renderer and trip React error #185
(Maximum update depth exceeded).
These tests do NOT reproduce the render storm (that requires real React
renders and real OS key-repeat timing the unit harness cannot simulate).
They guard the invariant the throttle must preserve: after a burst of
`move-selected` events, the final committed shape position equals exactly
`(event-count) * nudge-step` — i.e. the throttle drops no displacement.
The legacy (non-WASM) branch case specifically guards the new un-sampled
`rx/last` commit substream against `sample` dropping the tail value on
completion."
(:require
[app.common.geom.rect :as grc]
[app.common.test-helpers.compositions :as ctho]
[app.common.test-helpers.files :as cthf]
[app.common.test-helpers.ids-map :as cthi]
[app.common.test-helpers.shapes :as cths]
[app.main.data.workspace.selection :as dws]
[app.main.data.workspace.transforms :as dwt]
[beicon.v2.core :as rx]
[cljs.test :as t :include-macros true]
[frontend-tests.helpers.state :as ths]
[frontend-tests.helpers.wasm :as thw]))
(t/use-fixtures :each
{:before (fn []
(cthi/reset-idmap!)
;; The Node test environment has no real WASM binary, so the
;; WASM-branch cases below would throw inside `wasm.api/*`
;; calls. Install lightweight mocks that let the CLJS-side
;; logic (`apply-wasm-modifiers`'s `dwsh/update-shapes`)
;; run normally while stubbing the WASM heap boundary.
(thw/setup-wasm-mocks!))
:after (fn []
;; Teardown runs after the async test's `done` fires.
(thw/teardown-wasm-mocks!))})
(def ^:private nudge-event-count
"Number of `move-selected` events emitted in a burst. Chosen large enough
that, with a 16 ms `sample` cadence and the unit harness's synchronous
dispatch, the throttled live-preview substream would coalesce emissions
were it not for the final `rx/last` commit honoring the exact cumulative
position."
20)
(def ^:private run-store-timeout-ms
"Delay before `run-store` invokes its completion callback.
`nudge-selected-shapes` does not complete its transform streams on source
completion alone; its internal stopper fires either after 1000 ms of idle
time or 250 ms after a key-up event (see `transforms.cljs`). The unit
harness emits no keyboard events, so only the 1000 ms idle timer can fire.
We let the harness wait long enough for that timer to fire and run the
final `apply-modifiers`/`finish-transform` before reading the store."
1500)
(defn- run-nudge-store
"Like `ths/run-store` but the outer subscription completes on a fixed
timer instead of immediately on `:the/end`, giving the nudge's idle
stopper time to fire and commit the cumulative displacement."
[store done events completed-cb]
(ths/run-store store done events completed-cb (fn [_stream] (rx/timer run-store-timeout-ms))))
(defn- shape-x
"Read a shape's committed x position from its points-derived rect."
[file label]
(let [shape (cths/get-shape file label)
rect (grc/points->rect (:points shape))]
(:x rect)))
(defn- shape-y
"Read a shape's committed y position from its points-derived rect."
[file label]
(let [shape (cths/get-shape file label)
rect (grc/points->rect (:points shape))]
(:y rect)))
(defn- make-file
[]
(-> (cthf/sample-file :file1)
(ctho/add-rect :rect1 :x 100 :y 200 :width 50 :height 50)))
(defn- burst-move-events
"Builds the event sequence: select rect, then `n` `move-selected` events
in the given `direction` (with `shift?` controlling the nudge big/small
step size — false = 1 px, true = 10 px)."
[file direction shift? n]
(let [rect (cths/get-shape file :rect1)]
(into [(dws/select-shape (:id rect))]
(repeat n (dwt/move-selected direction shift?)))))
;; ---------------------------------------------------------------------------
;; Default (WASM) branch — `render-wasm/v1` active (helpers/state default).
;; ---------------------------------------------------------------------------
(t/deftest nudge-burst-wasm-branches-commits-exact-final-position-right-small
(t/async
done
(let [file (make-file)
store (ths/setup-store file)
events (burst-move-events file :right false nudge-event-count)]
(run-nudge-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
final-x (shape-x file' :rect1)]
;; nudge small = 1 px, direction :right => +x.
(t/is (= (+ 100 nudge-event-count) final-x)
"WASM branch: final x equals original + N * small-nudge after a throttled burst")))))))
(t/deftest nudge-burst-wasm-branches-commits-exact-final-position-up-big
(t/async
done
(let [file (make-file)
store (ths/setup-store file)
events (burst-move-events file :up true nudge-event-count)]
(run-nudge-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
final-y (shape-y file' :rect1)]
;; nudge big = 10 px, direction :up => -y.
(t/is (= (- 200 (* 10 nudge-event-count)) final-y)
"WASM branch: final y equals original - N * big-nudge after a throttled burst")))))))
;; ---------------------------------------------------------------------------
;; Legacy (non-WASM) branch — :renderer :svg disables render-wasm/v1.
;; This case specifically guards the new `rx/last` commit substream added
;; alongside the live-preview sample: if `sample` drops the tail value on
;; completion, the un-sampled `rx/last` write still delivers the exact
;; cumulative position to `apply-modifiers`.
;; ---------------------------------------------------------------------------
(t/deftest nudge-burst-legacy-branch-commits-exact-final-position-right-small
(t/async
done
(let [file (make-file)
store (ths/setup-store file {:renderer :svg})
events (burst-move-events file :right false nudge-event-count)]
(run-nudge-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
final-x (shape-x file' :rect1)]
(t/is (= (+ 100 nudge-event-count) final-x)
"Legacy branch: final x equals original + N * small-nudge after a throttled burst")))))))
(t/deftest nudge-burst-legacy-branch-commits-exact-final-position-up-big
(t/async
done
(let [file (make-file)
store (ths/setup-store file {:renderer :svg})
events (burst-move-events file :up true nudge-event-count)]
(run-nudge-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
final-y (shape-y file' :rect1)]
(t/is (= (- 200 (* 10 nudge-event-count)) final-y)
"Legacy branch: final y equals original - N * big-nudge after a throttled burst")))))))

2
frontend/test/frontend_tests/runner.cljs
View File

@ -26,6 +26,7 @@
[frontend-tests.logic.copying-and-duplicating-test]
[frontend-tests.logic.frame-guides-test]
[frontend-tests.logic.groups-test]
[frontend-tests.logic.nudge-selected-shapes-test]
[frontend-tests.logic.pasting-in-containers-test]
[frontend-tests.main-errors-test]
[frontend-tests.plugins.comments-test]
@ -99,6 +100,7 @@
'frontend-tests.logic.copying-and-duplicating-test
'frontend-tests.logic.frame-guides-test
'frontend-tests.logic.groups-test
'frontend-tests.logic.nudge-selected-shapes-test
'frontend-tests.logic.pasting-in-containers-test
'frontend-tests.plugins.context-shapes-test
'frontend-tests.plugins.comments-test