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 b452a4fd8..9c3be50c7 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 @@ -178,7 +178,7 @@ describe('useCreateFeature (target: paper)', () => { }); }); - it('calls onUpdateFeature when dependencies change', async () => { + it('calls onUpdateFeature when dependencies change (not on initial mount)', async () => { const onUpdateFeature = jest.fn(); const mockInstance = { count: 0 }; @@ -201,15 +201,16 @@ describe('useCreateFeature (target: paper)', () => { const Wrapper = createPaperWrapper(); const { rerender } = render(, { wrapper: Wrapper }); + // onUpdateFeature should NOT be called on initial mount await waitFor(() => { - expect(onUpdateFeature).toHaveBeenCalled(); + expect(onUpdateFeature).not.toHaveBeenCalled(); }); - const callsAfterMount = onUpdateFeature.mock.calls.length; + // Trigger a dependency change rerender(); await waitFor(() => { - expect(onUpdateFeature.mock.calls.length).toBeGreaterThan(callsAfterMount); + expect(onUpdateFeature).toHaveBeenCalledTimes(1); expect(onUpdateFeature).toHaveBeenLastCalledWith( expect.objectContaining({ graphStore: expect.any(Object), @@ -225,6 +226,59 @@ describe('useCreateFeature (target: paper)', () => { // Graph target // ──────────────────────────────────────────────────────────────────────────── +// ──────────────────────────────────────────────────────────────────────────── +// onAddFeature call count +// ──────────────────────────────────────────────────────────────────────────── + +describe('onAddFeature is called exactly once', () => { + it('paper target: onAddFeature fires once on mount', async () => { + const onAddFeature = jest.fn(() => ({ + id: 'once-paper', + instance: { value: 1 }, + })); + + function TestComponent() { + useCreateFeature('paper', { id: 'once-paper', onAddFeature }); + return null; + } + + const Wrapper = createPaperWrapper(); + render(, { wrapper: Wrapper }); + + // Wait for feature to be registered + await waitFor(() => { + expect(onAddFeature).toHaveBeenCalled(); + }); + + expect(onAddFeature).toHaveBeenCalledTimes(1); + }); + + it('graph target: onAddFeature fires once on mount', async () => { + const onAddFeature = jest.fn(() => ({ + id: 'once-graph', + instance: { value: 1 }, + })); + + function TestComponent() { + useCreateFeature('graph', { id: 'once-graph', onAddFeature }); + return null; + } + + const Wrapper = createGraphWrapper(); + render(, { wrapper: Wrapper }); + + await waitFor(() => { + expect(onAddFeature).toHaveBeenCalled(); + }); + + expect(onAddFeature).toHaveBeenCalledTimes(1); + }); +}); + +// ──────────────────────────────────────────────────────────────────────────── +// Graph target +// ──────────────────────────────────────────────────────────────────────────── + describe('useCreateFeature (target: graph)', () => { it('registers a feature with the graph store', async () => { const mockInstance = { doSomething: jest.fn() }; @@ -332,7 +386,7 @@ describe('useCreateFeature (target: graph)', () => { }); }); - it('calls onUpdateFeature when dependencies change', async () => { + it('calls onUpdateFeature when dependencies change (not on initial mount)', async () => { const onUpdateFeature = jest.fn(); const mockInstance = { count: 0 }; @@ -355,15 +409,16 @@ describe('useCreateFeature (target: graph)', () => { const Wrapper = createGraphWrapper(); const { rerender } = render(, { wrapper: Wrapper }); + // onUpdateFeature should NOT be called on initial mount await waitFor(() => { - expect(onUpdateFeature).toHaveBeenCalled(); + expect(onUpdateFeature).not.toHaveBeenCalled(); }); - const callsAfterMount = onUpdateFeature.mock.calls.length; + // Trigger a dependency change rerender(); await waitFor(() => { - expect(onUpdateFeature.mock.calls.length).toBeGreaterThan(callsAfterMount); + expect(onUpdateFeature).toHaveBeenCalledTimes(1); expect(onUpdateFeature).toHaveBeenLastCalledWith( expect.objectContaining({ graphStore: expect.any(Object), diff --git a/packages/joint-react/src/hooks/use-create-features.ts b/packages/joint-react/src/hooks/use-create-features.ts index 25387f68a..eb8071142 100644 --- a/packages/joint-react/src/hooks/use-create-features.ts +++ b/packages/joint-react/src/hooks/use-create-features.ts @@ -235,13 +235,30 @@ export function useCreateFeature( const asChildren = !!paperStore; - // Create and register the feature + // Guard: skip onUpdateFeature on initial mount — it must only fire on dependency changes + const isMountedRef = useRef(false); + // Holds the created feature to survive strict-mode cleanup/re-mount without re-calling onAddFeature + const featureRef = useRef(null); + + // Create and register the feature (fires onAddFeature exactly once) useLayoutEffect(() => { if (isPaperTarget && !paperStore) return; + + // Re-register cached feature if it was removed by strict-mode cleanup + if (featureRef.current) { + registerFeature(target, graphStore, paperStore, featureRef.current); + setForwardRef(forwardedRef, featureRef.current.instance); + return () => unregisterFeature(target, graphStore, paperStore, featureRef.current!.id); + } + const feature = createAndRegisterFeature(target, onAddFeature, graphStore, paperStore, asChildren); + featureRef.current = feature; registerFeature(target, graphStore, paperStore, feature); setForwardRef(forwardedRef, feature.instance); - return () => unregisterFeature(target, graphStore, paperStore, feature.id); + return () => { + isMountedRef.current = false; + unregisterFeature(target, graphStore, paperStore, feature.id); + }; }, [graphStore, paperStore]); // Fire onLoad when feature instance becomes available @@ -252,16 +269,23 @@ export function useCreateFeature( fireOnLoad(target, onLoad, graphStore, paperStore, resolvedFeature, asChildren); }, [resolvedFeature]); - // Fire onUpdateFeature when dependencies change + // Fire onUpdateFeature ONLY when dependencies change after mount. + // Never fires on initial mount — onAddFeature handles creation. + // resolvedFeature is intentionally NOT in deps to prevent spurious fires + // when the feature instance first resolves (null → instance). useLayoutEffect(() => { if (!onUpdateFeature) return; if (isPaperTarget && !paperStore) return; + if (!isMountedRef.current) { + isMountedRef.current = true; + return; + } const existingFeature = resolveExistingFeature(target, graphStore, paperStore, id); fireOnUpdate(target, onUpdateFeature, graphStore, paperStore, existingFeature?.instance, asChildren); if (existingFeature) { registerFeature(target, graphStore, paperStore, existingFeature); } - }, [graphStore, resolvedFeature, paperStore, ...dependencies]); + }, [graphStore, paperStore, ...dependencies]); return featureContext; }