diff --git a/packages/eui/changelogs/upcoming/9356.md b/packages/eui/changelogs/upcoming/9356.md new file mode 100644 index 000000000000..e9fba406df94 --- /dev/null +++ b/packages/eui/changelogs/upcoming/9356.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- Fixed a potential crash in the flyout system: due to asynchronous state updates and React's batching behavior, it was possible to experience a crash when closing a managed flyout. diff --git a/packages/eui/src/components/flyout/manager/flyout_managed.test.tsx b/packages/eui/src/components/flyout/manager/flyout_managed.test.tsx index a7c4d36b9314..210649196972 100644 --- a/packages/eui/src/components/flyout/manager/flyout_managed.test.tsx +++ b/packages/eui/src/components/flyout/manager/flyout_managed.test.tsx @@ -8,9 +8,19 @@ /* eslint-disable @typescript-eslint/no-var-requires */ +jest.mock('react-dom', () => { + const actual = jest.requireActual('react-dom'); + const mockFlushSync = jest.fn((callback: () => void) => callback()); + return { + ...actual, + flushSync: mockFlushSync, + }; +}); + import React from 'react'; import { act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import * as ReactDOM from 'react-dom'; import { render } from '../../../test/rtl'; import { requiredProps } from '../../../test/required_props'; @@ -23,6 +33,8 @@ import { PROPERTY_LEVEL, } from './const'; +const mockFlushSync: jest.Mock = jest.mocked(ReactDOM.flushSync); + // Mock base flyout to a simple div to avoid complex internals jest.mock('../flyout.component', () => { const React = require('react'); @@ -46,12 +58,15 @@ jest.mock('../flyout.component', () => { // Shared mock functions - must be defined in module scope for Jest const mockCloseFlyout = jest.fn(); -const createMockState = () => ({ + +// Create mock state and functions once at module scope to avoid redundant object creation +const mockState = { sessions: [], flyouts: [], layoutMode: 'side-by-side' as const, -}); -const createMockFunctions = () => ({ +}; + +const mockFunctions = { dispatch: jest.fn(), addFlyout: jest.fn(), closeFlyout: mockCloseFlyout, @@ -60,14 +75,16 @@ const createMockFunctions = () => ({ goBack: jest.fn(), goToFlyout: jest.fn(), historyItems: [], -}); +}; + +const mockFlyoutManager = { + state: mockState, + ...mockFunctions, +}; // Mock hooks that would otherwise depend on ResizeObserver or animation timing jest.mock('./hooks', () => ({ - useFlyoutManager: () => ({ - state: createMockState(), - ...createMockFunctions(), - }), + useFlyoutManager: () => mockFlyoutManager, useIsFlyoutActive: () => true, useHasChildFlyout: () => false, useParentFlyoutSize: () => 'm', @@ -101,10 +118,7 @@ jest.mock('./validation', () => ({ jest.mock('./provider', () => ({ ...jest.requireActual('./provider'), - useFlyoutManager: () => ({ - state: createMockState(), - ...createMockFunctions(), - }), + useFlyoutManager: () => mockFlyoutManager, })); // Mock resize observer hook to return a fixed width @@ -112,6 +126,9 @@ jest.mock('../../observer/resize_observer', () => ({ useResizeObserver: () => ({ width: 480 }), })); +// Cache the actual validation module for tests that need to temporarily restore it +const actualValidation = jest.requireActual('./validation'); + describe('EuiManagedFlyout', () => { const renderInProvider = (ui: React.ReactElement) => render({ui}); @@ -369,13 +386,10 @@ describe('EuiManagedFlyout', () => { describe('size handling', () => { it('defaults main flyout size to "m" when no size is provided', () => { - // Import the real validation function to test the actual behavior - const { validateManagedFlyoutSize } = jest.requireActual('./validation'); - // Temporarily restore the real validation function for this test const originalMock = require('./validation').validateManagedFlyoutSize; require('./validation').validateManagedFlyoutSize = - validateManagedFlyoutSize; + actualValidation.validateManagedFlyoutSize; const { getByTestSubject } = renderInProvider( { }); it('defaults child flyout size to "s" when no size is provided', () => { - // Import the real validation function to test the actual behavior - const { validateManagedFlyoutSize } = jest.requireActual('./validation'); - // Temporarily restore the real validation function for this test const originalMock = require('./validation').validateManagedFlyoutSize; require('./validation').validateManagedFlyoutSize = - validateManagedFlyoutSize; + actualValidation.validateManagedFlyoutSize; const { getByTestSubject } = renderInProvider( { expect(mockCloseFlyout).toHaveBeenCalledWith('main-flyout'); expect(onCloseMain).toHaveBeenCalled(); }); + + it('uses flushSync to ensure synchronous state update before DOM cleanup', () => { + const onClose = jest.fn(); + + const { getByTestSubject } = renderInProvider( + + ); + + // Clear any setup calls + mockFlushSync.mockClear(); + mockCloseFlyout.mockClear(); + + // Trigger close via user interaction + act(() => { + userEvent.click(getByTestSubject('managed-flyout')); + }); + + // Verify flushSync was called + expect(mockFlushSync).toHaveBeenCalledTimes(1); + expect(mockFlushSync).toHaveBeenCalledWith(expect.any(Function)); + + // Verify closeFlyout was called (inside flushSync) + expect(mockCloseFlyout).toHaveBeenCalledWith('flush-sync-test'); + + // Verify onClose was called after the synchronous state update + expect(onClose).toHaveBeenCalled(); + }); + + it('calls closeFlyout inside flushSync callback', () => { + const onClose = jest.fn(); + const callOrder: string[] = []; + + // Track execution order + mockFlushSync.mockImplementation((callback: () => void) => { + callOrder.push('flushSync-start'); + callback(); + callOrder.push('flushSync-end'); + }); + + mockCloseFlyout.mockImplementation(() => { + callOrder.push('closeFlyout'); + }); + + onClose.mockImplementation(() => { + callOrder.push('onClose'); + }); + + const { getByTestSubject } = renderInProvider( + + ); + + // Clear setup + callOrder.length = 0; + + // Trigger close + act(() => { + userEvent.click(getByTestSubject('managed-flyout')); + }); + + // Verify closeFlyout is called INSIDE flushSync, and onClose is called AFTER + expect(callOrder).toEqual([ + 'flushSync-start', + 'closeFlyout', + 'flushSync-end', + 'onClose', + ]); + }); }); }); diff --git a/packages/eui/src/components/flyout/manager/flyout_managed.tsx b/packages/eui/src/components/flyout/manager/flyout_managed.tsx index 25d069c13fdb..ca23b4cac09a 100644 --- a/packages/eui/src/components/flyout/manager/flyout_managed.tsx +++ b/packages/eui/src/components/flyout/manager/flyout_managed.tsx @@ -6,7 +6,15 @@ * Side Public License, v 1. */ -import React, { useEffect, useMemo, useRef, useState, forwardRef } from 'react'; +import React, { + useEffect, + useLayoutEffect, + useMemo, + useRef, + useState, + forwardRef, +} from 'react'; +import { flushSync } from 'react-dom'; import { useCombinedRefs, useEuiMemoizedStyles } from '../../../services'; import { useEuiI18n } from '../../i18n'; import { useResizeObserver } from '../../observer/resize_observer'; @@ -168,29 +176,31 @@ export const EuiManagedFlyout = forwardRef( // Track if flyout was ever registered to avoid false positives on initial mount const wasRegisteredRef = useRef(false); - // Register with flyout manager context when open, remove when closed + // Track flyoutExistsInManager in a ref to avoid dependency loop + // The cleanup function needs the current value but shouldn't cause re-runs + const flyoutExistsInManagerRef = useRef(flyoutExistsInManager); + + // Update ref when flyoutExistsInManager changes useEffect(() => { + flyoutExistsInManagerRef.current = flyoutExistsInManager; + }, [flyoutExistsInManager]); + + // Register with flyout manager context when open, remove when closed + // Using useLayoutEffect to run synchronously before DOM updates + useLayoutEffect(() => { addFlyout(flyoutId, title!, level, size as string); return () => { // Only call closeFlyout if it wasn't already called via onClose // This prevents duplicate removal when using Escape/X button - if (flyoutExistsInManager) { + if (flyoutExistsInManagerRef.current) { closeFlyout(flyoutId); } // Reset navigation tracking when explicitly closed via isOpen=false wasRegisteredRef.current = false; }; - }, [ - flyoutId, - title, - level, - size, - addFlyout, - closeFlyout, - flyoutExistsInManager, - ]); + }, [flyoutId, title, level, size, addFlyout, closeFlyout]); // Detect when flyout has been removed from manager state (e.g., via Back button) // and trigger onClose callback to notify the parent component @@ -224,16 +234,6 @@ export const EuiManagedFlyout = forwardRef( } }, [currentSession, flyoutId, level]); - useEffect(() => { - return () => { - // Only remove from manager on component unmount if still registered - // This cleanup serves as a final safety net for edge cases - if (flyoutExistsInManager) { - closeFlyout(flyoutId); - } - }; - }, [closeFlyout, flyoutId, flyoutExistsInManager]); - // Track width changes for flyouts const { width } = useResizeObserver(isActive ? flyoutRef : null, 'width'); @@ -242,7 +242,10 @@ export const EuiManagedFlyout = forwardRef( // CRITICAL: Update manager state FIRST before allowing React to unmount // This prevents race conditions during portal → inline DOM transitions // and ensures cascade close logic runs before DOM cleanup begins - closeFlyout(flyoutId); + // Using flushSync to force synchronous state update completion + flushSync(() => { + closeFlyout(flyoutId); + }); // trigger parent callback, unmounts the component if (onCloseCallbackRef.current) {