Repository navigation
fix(nodes): a connection moved to an unused face has its handle measured before it is drawn (React Flow 008) - #356
Merged
Merged
Conversation
… before React Flow draws it Trial v2 of note 51's option 3 (share note 53), an implementation candidate, not yet a release. After a reversal or an end moved to a face no connection of that node used, React Flow drew the connection before the face's handle existed and measured: in development it warned (error 008) and for one animation-frame sample the path was missing from the DOM. The line was not missing from any painted frame in the measurement (headless, before and after), so this reduces a development warning and a transient DOM state; it is not a visual fix. - A node mounts the union of the handles its connections use in the document (graph store) and in React Flow's store: React Flow's `edges` prop reaches its store in a passive effect after the commit, so the document leads and the union keeps the old face until React Flow catches up. - A change that ADDS a handle is measured at once in the layout effect; a change that only removes handles is measured on the next animation frame as before. Measuring a removal at once ran React Flow's edge selectors of the previous render (useSyncExternalStore swaps them in a passive effect), which warned for the handle just removed. - At rest only the used faces stay mounted, as #350 made them. Measured on the small fixture, actions as a user does them (three runs each, positive screen controls passing): Reverse, its undo and redo, an end moved to an unused side from the Ends group and from the grip popover, and their undo / redo — no 008 warning and no DOM gap on this code; on main 1-3 warnings and one missing DOM sample per action, except the reversal's undo. MMO playback fps and node select / deselect time, interleaved with main: the ranges overlap (no difference this measurement resolves; not a proof of no cost).
…008 and never loses its path node-face-handles.spec.ts gains the regression check of share note 53. On the small fixture, as a user does it: Reverse, the To end moved to an unused side in the Ends group, an end moved to an unused side from the grip's popover, each with its undo and redo. It records React Flow's development warning (`Couldn't create edge for ... handle id`, error 008; the suite runs the dev server) and, every animation frame, whether the connection's path is in the DOM, and expects neither to happen. On main's nodes.tsx it fails: 8 animation frames without the path (the first assertion; the warnings are also raised there, measured in note 53). On this branch the spec passes, 4 tests. The change declaration `.changes/face-handle-measure.json` (internal): nothing a person sees or stores changes.
Deploying cozy-loop-studio with
|
| Latest commit: |
4743857
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00b87943.cozy-loop-studio.pages.dev |
| Branch Preview URL: | https://fix-face-handle-measure.cozy-loop-studio.pages.dev |
MerciHanrim
added a commit
that referenced
this pull request
Oct 17, 2026
* perf(nodes): a connection drag's start and end measure every node's face handles once Share note 58. While a connection or an end grip is dragged every node mounts its face handles, and at the end it unmounts the unused ones — every node, in the same commit. Each node then measured its handles on its own: one React Flow `updateNodeInternals` call per node, each a store update run through every subscriber (the 63-node gacha banner Template: 848 handles). Counted on main: 61-62 measurement calls per start and as many per-node frames per release. Now the first node whose layout effect sees the drag state change measures every node in ONE call (`src/components/nodes/faceBatch.ts`), and the others skip: - at the start, at once: React applies the whole commit's DOM before any layout effect runs, so every node's new handles are in the DOM, nothing is painted, and React Flow's edges have not caught up — the moment each node measured itself in before; - at the end, on the next animation frame (only removals), as one node's removal is (the React Flow 008 order of #356 is kept); - a new drag cancels an end's measurement still waiting. Every other mount change (selection, focus, a document change) is measured per node as before. `docs/edge-routing.md` §ER17.1 states the rules. This is option 1 of the four arms measured (main / 1 / 2 / 1 + 2); the code is the measured copy with the batch moved into its own module. Production build, two interleaved rounds, medians of eight drags (input event to the second animation frame): gacha start 113-117 -> 73-74 ms and release 123-132 -> 78-83 ms at CPU x1; 795-817 -> 371-377 ms and 674-712 -> 280-301 ms at x4; Coffee not slower. Measurement calls per start / release 61-62 -> 1; the store updates in those windows 64-65 -> 4 and 75-81 -> 15. Tests: - `faceBatch.test.ts` (6): one commit's requests are one measurement of every node, at once for a start, on the next frame for an end; a new drag cancels the waiting end; the nodes are the canvas's when it runs. Two deliberate mutations (no merging; no cancel) fail it. - `e2e/connection-draw-faces.spec.ts` (5): one drag can land on an unused face of every node (React Flow picks each from its measured bounds — a node left out of the measurement fails it, checked by mutation); a drag released and started again in one task, and five back to back, then a drop on a face; a refused drawing then the next connection; Esc on an end grip drag then the next connection; no 008 in the console throughout. Checks: tsc, lint, full vitest (3 598 passed, 1 skipped), e2e shards; dev, fresh server: the new spec and the 9 related specs, 109 passed. * chore(release): 0.28.4 — starting and finishing a connection responds faster - package.json / package-lock.json 0.28.4; `.changes/draw-start-batch.json` (user-facing, `release:0.28.4`, Lumi's classification 2026-10-17), date 2026-10-17, to be checked on the merge day. - What's new 0.28.4, three lines in 18 languages (Daisy's draft on Lumi's direction "the response when you start and finish drawing a connection is improved", for review; the measured reductions stay in the CHANGELOG and the technical record): starting to draw responds faster, especially in large diagrams; letting go too, made or dropped on the empty canvas, and dragging an end grip; nothing else changes, every side of every node is still offered. The release-note validator asks for 3 to 5 lines, so the one-sentence direction is three lines. Grip / canvas / side words follow each locale's 0.27.0 line and UI. es-ES = es-419 and pt-PT = pt-BR, written neutrally; French avoids the punctuation its spacing rule governs. Locale count pins follow (catalog 1148, runtime 1370); ru adds `создаётся` to its ё list; the e2e release list pins 0.28.4. - CHANGELOG v0.28.4 (what was slow, now, how much on one machine); README Latest v0.28.4 (v0.28.3 kept as a section, v0.28.0 moved into the CHANGELOG list). The translation contract tests pass: evidence of the catalogs' structure and terms, not a review of how the lines read; no native-speaker review. Checks: tsc, lint, full vitest (3 598 passed, 1 skipped), change declaration, i18n, i18n surface, control bytes, isolate / ICU arguments, locale lists, functional glyphs, e2e shards, snapshot policy, form / content direction, no pseudo-locales. * chore(release): What's new 0.28.4 says only what was measured and checked (Lumi's review) Lumi's review of the report and the measurement record (2026-10-17; not a code review of the product diff): - the second line said that a release which makes a connection, and an end grip drag, got faster; only a drag released on the empty canvas was timed (the others were checked to work, not timed), so it now says that one; - "Nothing else changes" was wider than what was checked; the third line is now the checked contract only: while drawing a connection, handles on every side of every node remain available. English and Korean in Lumi's words; the other 16 follow (the port / point word each locale's UI uses for a handle; pt says "arrastada" for both regions, the pt-PT audit refusing the estar + gerund form). README Latest and the CHANGELOG follow; the CHANGELOG says which gestures were timed. ru drops `создаётся` from its ё list again (the word left the line). No native-speaker review. Checks: tsc, lint, full vitest (3 598 passed, 1 skipped), change declaration, i18n, i18n surface, control bytes, isolate / ICU arguments, locale lists, functional glyphs; whats-new.spec.ts on a fresh dev server, 96 passed. The full local e2e is not rerun for this wording-only change (Lumi). * fix(nodes): the drag measurement's state is each canvas's, and a drag ends when React Flow's connection ends Found in my own review of the diff, on the points Lumi named for the code review (state per canvas; a waiting measurement on restart, document replacement and unmount). `faceBatch.ts` kept one module-wide flag "this drag is measured", turned on at a start and off at an end reported by a node. If no node saw the end — every node gone during the drag (its insert undone with Ctrl+Z, or the node structure switched) — the flag stayed on, and the NEXT drag's start was taken for "already measured": no node's faces were measured, so React Flow could not find an unused face to drop on. Reproduced before the fix in a unit test and in an e2e (a module inserted into an empty document, a drag from one of its nodes, Ctrl+Z while held, release, Ctrl+Y, a new drag: the target's unused face never gets `connectingto`). Now: - a drag also ends when React Flow's `connection.inProgress` goes false (a store subscription, one per canvas), so the next start is measured even when no node saw the end; the end seen by the store and by the nodes is one measurement; - the state is each canvas's, keyed by the store's own `getState`. `useStoreApi()` gives every component its own `{ getState, setState, subscribe }` wrapper (memoized per hook call): keyed by the wrapper, every node had its own state and nothing was merged — a first attempt keyed that way measured all 63 gacha nodes 61 times per start (counted) and made a start about 3 times and a release about 6 times slower than main (production, x1); never committed; - unchanged: a start measures every node at once, an end on the next frame, a start cancels an end's frame that has not run. A frame is not cancelled when the canvas goes away; it runs, reads the store and the DOM then, finds no `domNode` (React Flow resets its store on unmount) and measures nothing. Counted again (dev, diagnostic copies, never in a timing run): one measurement of every node per start and per release, as before the fix. Tests: - `faceBatch.test.ts` 11 (was 6): + requests of one start in separate tasks are still one measurement; + an end seen by React Flow and by the nodes is one measurement; + a drag whose end no node reported is over when React Flow ends it; + every node's own wrapper of one store is one canvas; + two canvases are measured apart; + an end with no drag begun measures nothing. The test-only reset is gone (each test makes its own store). The module-wide version fails 6 of them, the per-task (microtask) version 3, the wrapper-keyed version 1. - `e2e/connection-draw-faces.spec.ts` 6 (was 5): + every node gone during a drag (its insert undone) does not stop the next drag measuring every node; it fails on the previous faceBatch and passes now. After the redo a click on the empty pane did not clear the restored selection there (not traced, left open), so the test clears it through the store; `emptySpot` now looks for a point where the pane is on top. - `docs/edge-routing.md` §ER17.1 says how a drag ends and what the state is keyed by. Production timing of this code against main (base twice, this twice, interleaved, medians of eight drags): gacha start 86.8-92.2 -> 62.8-62.9 ms and release 85.3-87.6 -> 61.6-64.4 ms at CPU x1; 502.7-506.0 -> 285.6-291.1 ms and 438.4-471.3 -> 213.4-228.1 ms at x4; Coffee not slower. Checks: tsc, lint, full vitest (3 603 passed, 1 skipped), e2e shards, control bytes; dev, fresh server: the new spec and the 9 related specs, 110 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A connection moved to an unused face: its handle is mounted and measured before React Flow draws it
An internal fix (share notes 51 and 53). It reduces React Flow's development warning 008 and a transient DOM state; it is not a visual fix. No version, no What's new line.
What was wrong
After a reversal, or an end moved to a face no connection of that node used, React Flow drew the connection before the face's handle existed and was measured: in a development build it warned (
Couldn't create edge for … handle id, error 008), and for one animation-frame sample the connection's path was missing from the DOM. A production build prints nothing (React Flow's warning is a no-op there). In a headless measurement with a screenshot of every composited frame, checked by positive controls (one painted frame without the line is seen as one), no painted frame without the line was observed — on main or with this change.The change (
ab4323e,src/components/nodes/nodes.tsx)edgesprop reaches its store in a passive effect after the commit, so the document leads and the union keeps the old face until React Flow catches up.useSyncExternalStoreswaps them in a passive effect), which warned for the handle just removed — the first trial did that, the trace is in note 53.Measured (note 53)
On the small fixture, actions as a user does them, three runs each: Reverse, its undo and redo; the To end moved to an unused side from the Ends group, and its undo and redo; an end moved to an unused side from the grip's popover, and its undo and redo. On main: 1–3 warnings and one missing DOM sample per action (none on the reversal's undo). With this change: no warning, no missing DOM sample, in every action measured.
Cost, Early MMO, interleaved with main, three runs each: playback frame rate at CPU × 1 and × 2 and node select / deselect time, in its opening view and with the whole graph in view — the ranges overlap (for example × 2 with the whole graph in view: main 37.0–38.0 fps, this change 36.1–37.8). That is no difference this measurement resolves, not a proof of no cost: one document, two views, one viewport, headless Chromium on one machine; not measured: dragging, panning and zooming a large graph, drawing a connection (every face mounts then, now measured at once), a phone.
Starting to draw a connection (after the review)
Starting a drag from a handle mounts every face on every node, and with this change each node measures that mount at once. Measured on ordinary user Templates opened from the menu (Coffee, 23 nodes, 189 face handles mounted mid-drag; the gacha banner comparison, 63 nodes, 636, unlocked first), eight drags to the empty canvas per row, interleaved with main, three runs each; medians' ranges, from the first pointer move of the drag (or the release) to the second animation frame after it:
The start was not slower in any row, and faster on the gacha banner in every run (the cause was not traced). The release on Coffee at × 4 was slower in every run, by about 5–35 ms on medians of 354–390 ms; the other release rows overlap; that difference was not traced. On both main and this change, starting or ending a drag on the gacha banner takes about 0.2 s at × 1 and about 1.5–2 s at × 4: mounting every face on every node while connecting is costly by itself — a separate item, older than this change.
Test (
4743857)node-face-handles.spec.ts"a connection moved to an unused face is drawn on its handle at once": the three moves above with their undo and redo, on the dev server, expecting no 008 warning and no animation frame without the path. On main'snodes.tsxit fails on the DOM assertion (8 frames without the path), which comes first, so that run does not reach the warning assertion; the warnings on main are shown by the separate measurements above (note 53), not by this test's run on main. Here the spec passes (4 tests)..changes/face-handle-measure.json(internal).Verification
ab4323e--workers=1 --retries=0ab4323eab4323enode-face-handles,connection-reverse,connection-ends4743857node-face-handles.spec.ts4743857distconfig, thenpwaconfig, run separately4743857check:change-declaration,check:e2e-shards,check:control-bytes47438574743857adds only the test and the declaration to the head the full e2e ran on.Seen, not in this change
After undo / redo of a reversal or an end move, the Inspector's selection is empty but React Flow keeps the connection marked selected, so a click on it does not select it until the empty canvas is clicked (main too; note 53). A separate item.