diff --git a/packages/joint-react/jest.react18.config.mjs b/packages/joint-react/jest.react18.config.mjs new file mode 100644 index 0000000000..220fc13b04 --- /dev/null +++ b/packages/joint-react/jest.react18.config.mjs @@ -0,0 +1,26 @@ +// Full joint-react suite under React 18. The package develops on React 19, but +// the library supports React 18+ and some StrictMode timing differs between the +// two (e.g. the feature-creation double-render leak in use-create-features.ts). +// This runs the `default` project with React resolved to an aliased React 18 +// install (devDeps `react18` / `react18-dom`). The `react-compiler` project is +// React-19-specific (babel-plugin-react-compiler target '19'), so it is skipped. +// +// yarn test:react18 (this) +// yarn test:react19 (default jest, React 19) +import base from './jest.config.js'; + +const react18Map = { + '^react$': '/node_modules/react18', + '^react-dom(.*)$': '/node_modules/react18-dom$1', + '^react/(.*)$': '/node_modules/react18/$1', +}; + +export default { + projects: base.projects + .filter((project) => project.displayName !== 'react-compiler') + .map((project) => ({ + ...project, + displayName: `${project.displayName}-react18`, + moduleNameMapper: { ...project.moduleNameMapper, ...react18Map }, + })), +}; diff --git a/packages/joint-react/knip.json b/packages/joint-react/knip.json index 075f189f87..e2016b1f23 100644 --- a/packages/joint-react/knip.json +++ b/packages/joint-react/knip.json @@ -7,7 +7,9 @@ "@babel/preset-env", "@babel/preset-react", "@babel/preset-typescript", - "babel-plugin-react-compiler" + "babel-plugin-react-compiler", + "react18", + "react18-dom" ], "ignoreExportsUsedInFile": false } diff --git a/packages/joint-react/package.json b/packages/joint-react/package.json index 10dac90842..93c8fd2e7e 100644 --- a/packages/joint-react/package.json +++ b/packages/joint-react/package.json @@ -44,7 +44,9 @@ "build": "rollup -c rollup.config.ts --configPlugin esbuild", "build-storybook": "storybook build", "storybook": "storybook dev -p 6006", - "test": "yarn run typecheck && yarn run lint && yarn run knip && yarn jest", + "test": "yarn run typecheck && yarn run lint && yarn run knip && yarn run test:react19 && yarn run test:react18", + "test:react19": "jest", + "test:react18": "jest --config jest.react18.config.mjs", "typecheck": "tsc --noEmit", "lint": "eslint \"**/*.{ts,tsx}\"", "lint-fix": "yarn run lint --fix", @@ -120,6 +122,8 @@ "react-dom": "^19.1.1", "react-redux": "^9.2.0", "react-scan": "^0.4.3", + "react18": "npm:react@18.3.1", + "react18-dom": "npm:react-dom@18.3.1", "redux": "^5.0.1", "redux-undo": "1.1.0", "rollup": "^4.50.0", diff --git a/packages/joint-react/src/hooks/__tests__/use-create-features.test.tsx b/packages/joint-react/src/hooks/__tests__/use-create-features.test.tsx index 2366f07e98..a77d48b878 100644 --- a/packages/joint-react/src/hooks/__tests__/use-create-features.test.tsx +++ b/packages/joint-react/src/hooks/__tests__/use-create-features.test.tsx @@ -1,5 +1,7 @@ /* eslint-disable react-perf/jsx-no-new-object-as-prop */ +import { StrictMode } from 'react'; import { render, waitFor } from '@testing-library/react'; +import type { dia } from '@joint/core'; import { GraphProvider, Paper } from '../../components'; import { FeaturesProvider } from '../../components/features-provider/features-provider'; import { ELEMENT_MODEL_TYPE } from '../../mvc/element-model'; @@ -187,4 +189,51 @@ describe('useCreateFeature — paper target lifecycle', () => { expect(onAdd).toHaveBeenCalled(); }); }); + + it('does not leak a duplicate paper feature under StrictMode (React 18 double-render)', async () => { + // Regression for the selection "2x drag" bug. Under React 18's StrictMode + // the render body runs twice; the feature must still be created and bound + // EXACTLY once. A leaked duplicate binds a second paper listener (its + // constructor's side effect) that is never cleaned, so every interaction + // fires twice. Passes trivially on React 19 (single render) — the guard + // bites under the React 18 project (`yarn test:react18`). + let capturedPaper: dia.Paper | null = null; + const onAdd = jest.fn(({ paperStore }: { paperStore: { paper: dia.Paper } }) => { + const { paper } = paperStore; + capturedPaper = paper; + // Distinct per-instance listener on purpose: a shared handler would be + // removed for every instance on the first clean, masking a leaked duplicate. + // eslint-disable-next-line unicorn/consistent-function-scoping + const handler = () => {}; + paper.on('leak:probe', handler); + return { + id: 'leak-probe', + instance: { tag: 'leak' } as FeatureInstance, + clean() { + paper.off('leak:probe', handler); + }, + }; + }); + + render( + + + + +
leak-child
+ + + + + ); + + await waitFor(() => expect(capturedPaper).not.toBeNull()); + await waitFor(() => { + const paper = capturedPaper as unknown as { + _events?: Record; + } | null; + const listeners = paper?._events?.['leak:probe'] ?? []; + expect(listeners).toHaveLength(1); + }); + }); }); diff --git a/packages/joint-react/src/hooks/__tests__/use-reset-cells-ghost.test.tsx b/packages/joint-react/src/hooks/__tests__/use-reset-cells-ghost.test.tsx new file mode 100644 index 0000000000..801b01bfef --- /dev/null +++ b/packages/joint-react/src/hooks/__tests__/use-reset-cells-ghost.test.tsx @@ -0,0 +1,77 @@ +/* eslint-disable react-perf/jsx-no-new-function-as-prop */ +import { render, renderHook, act, waitFor, fireEvent } from '@testing-library/react'; +import { GraphProvider } from '../../components'; +import { graphProviderWrapper } from '../../utils/test-wrappers'; +import { useGraph } from '../use-graph'; +import { useCells } from '../use-cells'; +import { useSetCell, useResetCells } from '../use-cell-setters'; +import { useGraphStore } from '../use-graph-store'; +import { ELEMENT_MODEL_TYPE } from '../../mvc/element-model'; +import type { CellRecord } from '../../types/cell.types'; + +const initial: readonly CellRecord[] = [ + { id: 'a', type: ELEMENT_MODEL_TYPE, position: { x: 0, y: 0 }, size: { width: 10, height: 10 } } as CellRecord, + { id: 'b', type: ELEMENT_MODEL_TYPE, position: { x: 50, y: 0 }, size: { width: 10, height: 10 } } as CellRecord, +]; + +const extra: CellRecord = { + id: 'extra', + type: ELEMENT_MODEL_TYPE, + position: { x: 200, y: 0 }, + size: { width: 10, height: 10 }, +} as CellRecord; + +// Regression: `resetCells` calls `graph.resetCells()`, whose bulk `reset` event +// emits only `add`s for the surviving cells and no per-cell `remove`s. The +// reactive container must still drop the cells the reset removed — otherwise +// `useCells()` keeps counting ghost cells the canvas (rendered from the graph) +// no longer shows. +describe('resetCells reactive-container reconciliation', () => { + it('useCells((cells) => cells.length) reflects resetCells, not a stale ghost count', async () => { + function CountProbe() { + const { setCell, resetCells } = useGraph(); + const count = useCells((cells) => cells.length); + return ( +
+ {count} + + +
+ ); + } + + const { getByTestId } = render( + + + + ); + + await waitFor(() => expect(getByTestId('count').textContent).toBe('2')); + act(() => fireEvent.click(getByTestId('add'))); + await waitFor(() => expect(getByTestId('count').textContent).toBe('3')); + + // After reset the count must return to 2 — the removed 'extra' is gone. + act(() => fireEvent.click(getByTestId('reset'))); + await waitFor(() => expect(getByTestId('count').textContent).toBe('2')); + }); + + it('drops cells removed by resetCells from the reactive container (no ghosts)', async () => { + const wrapper = graphProviderWrapper({ initialCells: initial }); + const { result } = renderHook( + () => ({ setCell: useSetCell(), resetCells: useResetCells(), store: useGraphStore() }), + { wrapper } + ); + + act(() => result.current.setCell(extra)); + await waitFor(() => { + expect(result.current.store.graphProjection.cells.getAll()).toHaveLength(3); + }); + + act(() => result.current.resetCells(initial)); + await waitFor(() => { + const ids = result.current.store.graphProjection.cells.getAll().map((cell) => cell.id); + expect(ids).toHaveLength(2); + expect(ids).not.toContain('extra'); + }); + }); +}); diff --git a/packages/joint-react/src/hooks/use-create-features.ts b/packages/joint-react/src/hooks/use-create-features.ts index 94699b8524..f33baa573d 100644 --- a/packages/joint-react/src/hooks/use-create-features.ts +++ b/packages/joint-react/src/hooks/use-create-features.ts @@ -302,30 +302,38 @@ export function useCreateFeature( // Holds the created feature to survive strict-mode cleanup/re-mount without re-calling onAddFeature const featureRef = useRef(null); - // Paper-specific registration paths: - // - Paper not yet mounted: defer via `featureContext` so Paper's mount - // effect picks up the feature before it sets `isReady=true`. - // - Paper already mounted (this hook is being called from inside - // ``'s subtree): register synchronously during render so the - // feature is visible to sibling consumers reading `paperStore.features` - // in the same commit. Without this, a `` sibling that creates - // its underlying instance in `useImperativeApi`'s onLoad sees an empty - // feature snapshot and never picks up the inside-Paper-registered - // feature. - if (isPaperTarget && !featureContext.features.has(id) && !featureRef.current) { - if (paperStore) { - const feature = createAndRegisterFeature( - target, - onAddFeature, - graphStore, - paperStore, - true - ); - featureRef.current = feature; - registerFeature(target, graphStore, paperStore, feature); - } else { + // Register the feature synchronously DURING RENDER when the hook runs inside + // an already-mounted ``, so the feature is visible to sibling consumers + // reading `paperStore.features` in the SAME commit. Without this, a `` + // sibling that builds its instance in `useImperativeApi`'s onLoad sees an empty + // snapshot and never picks up the inside-Paper feature. When Paper is not yet + // mounted, the factory is deferred instead for Paper's mount effect to run. + const registerFeatureDuringRender = () => { + // Paper not mounted yet: defer the factory; Paper's mount effect picks it up. + if (!paperStore) { featureContext.features.set(id, onAddFeature); + return; + } + // StrictMode double-invokes render, and ref writes from the discarded first + // pass are NOT preserved into the second, so `featureRef.current` alone can't + // dedupe across the two passes (React 18 runs this block twice). The store + // registration IS preserved (external state), so adopt an instance already + // registered for this id instead of constructing a duplicate — a duplicate + // leaks (its constructor bound paper listeners; only the committed instance's + // cleanup runs) and fires every interaction twice (e.g. selection drag + // translating each cell 2x). + const alreadyRegistered = resolveExistingFeature(target, graphStore, paperStore, id); + if (alreadyRegistered) { + featureRef.current = alreadyRegistered; + return; } + const feature = createAndRegisterFeature(target, onAddFeature, graphStore, paperStore, true); + featureRef.current = feature; + registerFeature(target, graphStore, paperStore, feature); + }; + + if (isPaperTarget && !featureContext.features.has(id) && !featureRef.current) { + registerFeatureDuringRender(); } // Create and register the feature (fires onAddFeature exactly once) diff --git a/packages/joint-react/src/store/graph-changes.ts b/packages/joint-react/src/store/graph-changes.ts index 36baed6568..22a6d2d1e1 100644 --- a/packages/joint-react/src/store/graph-changes.ts +++ b/packages/joint-react/src/store/graph-changes.ts @@ -52,6 +52,12 @@ interface OnChangeOptions { readonly isInsideBatch: boolean; /** Skip the container commit; the open batch flushes it once, on close. */ readonly deferCommit: boolean; + /** + * The changes are a full bulk `reset`: `changes` holds every surviving cell + * as an `add`, so the consumer must prune any container cell not present here + * (a `reset` emits no per-cell `remove` events for the cells it drops). + */ + readonly isReset?: boolean; } interface Options { @@ -188,6 +194,7 @@ export function graphChanges(options: Options) { changes, isInsideBatch: isInsideBatch(), deferCommit: isDeferring(), + isReset: true, }); } ); diff --git a/packages/joint-react/src/store/graph-projection.ts b/packages/joint-react/src/store/graph-projection.ts index a4b9bff8fe..7c83882c00 100644 --- a/packages/joint-react/src/store/graph-projection.ts +++ b/packages/joint-react/src/store/graph-projection.ts @@ -68,7 +68,7 @@ export function graphProjection< const graphChangesController = graphChanges({ graph, onElementsSizeChange, - onChanges: ({ changes, isInsideBatch, deferCommit }) => { + onChanges: ({ changes, isInsideBatch, deferCommit, isReset }) => { for (const [id, change] of changes) { const { data, type } = change; switch (type) { @@ -108,6 +108,25 @@ export function graphProjection< } } + // A bulk `reset` sends only `add`s for the surviving cells and no per-cell + // `remove`s, so prune any container cell the reset dropped — otherwise + // reactive readers (`useCells`) keep counting ghost cells the canvas + // (rendered from the graph) no longer shows. Same reconciliation the + // React-driven `updateGraph()` path does below, keyed off the reset's set. + if (isReset) { + const survivingIds = new Set(changes.keys()); + // Snapshot ids first — `cells.delete` swap-pops the live array, so + // iterating `getAll()` while deleting would skip entries. + const staleIds: CellId[] = []; + for (const item of cells.getAll()) { + if (item.id !== undefined && !survivingIds.has(item.id)) staleIds.push(item.id); + } + for (const staleId of staleIds) { + cells.delete(staleId); + if (trackChanges) removed!.add(staleId); + } + } + // Two commit modes, decided by `deferCommit` (see graph-changes): // // - Plain batches (interactive drags, `auto-size`, layout) and lone edits diff --git a/yarn.lock b/yarn.lock index bf3e56acd3..25402424d0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6537,6 +6537,8 @@ __metadata: react-dom: "npm:^19.1.1" react-redux: "npm:^9.2.0" react-scan: "npm:^0.4.3" + react18: "npm:react@18.3.1" + react18-dom: "npm:react-dom@18.3.1" redux: "npm:^5.0.1" redux-undo: "npm:1.1.0" rollup: "npm:^4.50.0" @@ -22689,7 +22691,7 @@ __metadata: languageName: node linkType: hard -"loose-envify@npm:^1.4.0": +"loose-envify@npm:^1.1.0, loose-envify@npm:^1.4.0": version: 1.4.0 resolution: "loose-envify@npm:1.4.0" dependencies: @@ -26456,6 +26458,27 @@ __metadata: languageName: node linkType: hard +"react18-dom@npm:react-dom@18.3.1": + version: 18.3.1 + resolution: "react-dom@npm:18.3.1" + dependencies: + loose-envify: "npm:^1.1.0" + scheduler: "npm:^0.23.2" + peerDependencies: + react: ^18.3.1 + checksum: 10/3f4b73a3aa083091173b29812b10394dd06f4ac06aff410b74702cfb3aa29d7b0ced208aab92d5272919b612e5cda21aeb1d54191848cf6e46e9e354f3541f81 + languageName: node + linkType: hard + +"react18@npm:react@18.3.1": + version: 18.3.1 + resolution: "react@npm:18.3.1" + dependencies: + loose-envify: "npm:^1.1.0" + checksum: 10/261137d3f3993eaa2368a83110466fc0e558bc2c7f7ae7ca52d94f03aac945f45146bd85e5f481044db1758a1dbb57879e2fcdd33924e2dde1bdc550ce73f7bf + languageName: node + linkType: hard + "react@npm:19.1.1": version: 19.1.1 resolution: "react@npm:19.1.1" @@ -27968,6 +27991,15 @@ __metadata: languageName: node linkType: hard +"scheduler@npm:^0.23.2": + version: 0.23.2 + resolution: "scheduler@npm:0.23.2" + dependencies: + loose-envify: "npm:^1.1.0" + checksum: 10/e8d68b89d18d5b028223edf090092846868a765a591944760942b77ea1f69b17235f7e956696efbb62c8130ab90af7e0949bfb8eba7896335507317236966bc9 + languageName: node + linkType: hard + "scheduler@npm:^0.26.0": version: 0.26.0 resolution: "scheduler@npm:0.26.0"