Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/joint-react/jest.react18.config.mjs
Original file line number Diff line number Diff line change
@@ -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$': '<rootDir>/node_modules/react18',
'^react-dom(.*)$': '<rootDir>/node_modules/react18-dom$1',
'^react/(.*)$': '<rootDir>/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 },
})),
};
4 changes: 3 additions & 1 deletion packages/joint-react/knip.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
6 changes: 5 additions & 1 deletion packages/joint-react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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(
<StrictMode>
<GraphProvider initialCells={initialCells}>
<Paper style={{ width: 100, height: 100 }} id="leak-paper" renderElement={noopRender}>
<FeaturesProvider target="paper" id="leak-probe" onAddFeature={onAdd}>
<div>leak-child</div>
</FeaturesProvider>
</Paper>
</GraphProvider>
</StrictMode>
);

await waitFor(() => expect(capturedPaper).not.toBeNull());
await waitFor(() => {
const paper = capturedPaper as unknown as {
_events?: Record<string, readonly unknown[]>;
} | null;
const listeners = paper?._events?.['leak:probe'] ?? [];
expect(listeners).toHaveLength(1);
});
});
});
Original file line number Diff line number Diff line change
@@ -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 (
<div>
<span data-testid="count">{count}</span>
<button data-testid="add" onClick={() => setCell(extra)}>add</button>
<button data-testid="reset" onClick={() => resetCells(initial)}>reset</button>
</div>
);
}

const { getByTestId } = render(
<GraphProvider initialCells={initial}>
<CountProbe />
</GraphProvider>
);

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');
});
});
});
52 changes: 30 additions & 22 deletions packages/joint-react/src/hooks/use-create-features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,30 +302,38 @@ export function useCreateFeature<T>(
// Holds the created feature to survive strict-mode cleanup/re-mount without re-calling onAddFeature
const featureRef = useRef<Feature | null>(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
// `<Paper>`'s subtree): register synchronously during render so the
// feature is visible to sibling consumers reading `paperStore.features`
// in the same commit. Without this, a `<Stencil>` 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 `<Paper>`, so the feature is visible to sibling consumers
// reading `paperStore.features` in the SAME commit. Without this, a `<Stencil>`
// 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)
Expand Down
7 changes: 7 additions & 0 deletions packages/joint-react/src/store/graph-changes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -188,6 +194,7 @@ export function graphChanges(options: Options) {
changes,
isInsideBatch: isInsideBatch(),
deferCommit: isDeferring(),
isReset: true,
});
}
);
Expand Down
21 changes: 20 additions & 1 deletion packages/joint-react/src/store/graph-projection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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<CellId>(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
Expand Down
34 changes: 33 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
Loading