Skip to content

Commit fe52040

Browse files
tsullivantsullivan
committed
Fix infinite recursion and performance issues in flyout layout mode hook
Resize listener always runs, but expensive layout calculations only run when needed.
1 parent d15b754 commit fe52040

File tree

2 files changed

+182
-41
lines changed

2 files changed

+182
-41
lines changed

‎packages/eui/src/components/flyout/manager/layout_mode.test.tsx‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,152 @@ describe('layout_mode', () => {
616616
// The hook should re-evaluate with new window width
617617
expect(mockWindow.requestAnimationFrame).toHaveBeenCalled();
618618
});
619+
620+
describe('resize listener optimization', () => {
621+
it('should attach resize listener when there is a parent flyout', () => {
622+
// Set up session with only parent flyout (no child)
623+
mockUseCurrentSession.mockReturnValue({
624+
mainFlyoutId: 'main-1',
625+
childFlyoutId: null,
626+
});
627+
628+
mockUseCurrentChildFlyout.mockReturnValue(null);
629+
630+
const mockDispatch = jest.fn();
631+
mockUseFlyoutManager.mockReturnValue({
632+
state: {
633+
layoutMode: LAYOUT_MODE_SIDE_BY_SIDE,
634+
},
635+
dispatch: mockDispatch,
636+
});
637+
638+
render(<TestComponent />);
639+
640+
// Resize listener SHOULD be attached when parent flyout exists
641+
// to keep window dimensions fresh for when child opens
642+
expect(mockWindow.addEventListener).toHaveBeenCalledWith(
643+
'resize',
644+
expect.any(Function)
645+
);
646+
});
647+
648+
it('should attach resize listener when there is a child flyout', () => {
649+
// Set up session with both parent and child flyouts
650+
mockUseCurrentSession.mockReturnValue({
651+
mainFlyoutId: 'main-1',
652+
childFlyoutId: 'child-1',
653+
});
654+
655+
mockUseCurrentChildFlyout.mockReturnValue({
656+
flyoutId: 'child-1',
657+
level: 'child',
658+
size: 's',
659+
});
660+
661+
const mockDispatch = jest.fn();
662+
mockUseFlyoutManager.mockReturnValue({
663+
state: {
664+
layoutMode: LAYOUT_MODE_SIDE_BY_SIDE,
665+
},
666+
dispatch: mockDispatch,
667+
});
668+
669+
render(<TestComponent />);
670+
671+
// Resize listener SHOULD be attached when there's a child flyout
672+
expect(mockWindow.addEventListener).toHaveBeenCalledWith(
673+
'resize',
674+
expect.any(Function)
675+
);
676+
});
677+
678+
it('should add and remove resize listener when parent flyout opens and closes', () => {
679+
// Start with no flyouts at all
680+
mockUseCurrentSession.mockReturnValue({
681+
mainFlyoutId: null,
682+
childFlyoutId: null,
683+
});
684+
685+
mockUseCurrentMainFlyout.mockReturnValue(null);
686+
mockUseCurrentChildFlyout.mockReturnValue(null);
687+
688+
const mockDispatch = jest.fn();
689+
mockUseFlyoutManager.mockReturnValue({
690+
state: {
691+
layoutMode: LAYOUT_MODE_SIDE_BY_SIDE,
692+
},
693+
dispatch: mockDispatch,
694+
});
695+
696+
const { rerender } = render(<TestComponent />);
697+
698+
// Should have resize listener before flyout appears so that the initial layout mode is correct
699+
expect(mockWindow.addEventListener).toHaveBeenCalledWith(
700+
'resize',
701+
expect.any(Function)
702+
);
703+
704+
// Now open a parent flyout
705+
mockUseCurrentSession.mockReturnValue({
706+
mainFlyoutId: 'main-1',
707+
childFlyoutId: null,
708+
});
709+
710+
mockUseCurrentMainFlyout.mockReturnValue({
711+
flyoutId: 'main-1',
712+
level: 'main',
713+
size: 'm',
714+
});
715+
716+
rerender(<TestComponent />);
717+
718+
// Should now have attached the listener
719+
expect(mockWindow.addEventListener).toHaveBeenCalledWith(
720+
'resize',
721+
expect.any(Function)
722+
);
723+
724+
// Now close the parent flyout
725+
mockUseCurrentSession.mockReturnValue({
726+
mainFlyoutId: null,
727+
childFlyoutId: null,
728+
});
729+
730+
mockUseCurrentMainFlyout.mockReturnValue(null);
731+
732+
rerender(<TestComponent />);
733+
734+
// Should not remove the listener
735+
expect(mockWindow.removeEventListener).not.toHaveBeenCalled();
736+
});
737+
738+
it('should NOT attach resize listener when there is no parent flyout', () => {
739+
// Set up session with no flyouts at all
740+
mockUseCurrentSession.mockReturnValue({
741+
mainFlyoutId: null,
742+
childFlyoutId: null,
743+
});
744+
745+
mockUseCurrentMainFlyout.mockReturnValue(null);
746+
mockUseCurrentChildFlyout.mockReturnValue(null);
747+
748+
const mockDispatch = jest.fn();
749+
mockUseFlyoutManager.mockReturnValue({
750+
state: {
751+
layoutMode: LAYOUT_MODE_SIDE_BY_SIDE,
752+
},
753+
dispatch: mockDispatch,
754+
});
755+
756+
render(<TestComponent />);
757+
758+
// Should have resize listener before flyout appears so that the initial layout mode is correct
759+
expect(mockWindow.addEventListener).toHaveBeenCalledWith(
760+
'resize',
761+
expect.any(Function)
762+
);
763+
});
764+
});
619765
});
620766

621767
describe('useApplyFlyoutLayoutMode with fill size', () => {

‎packages/eui/src/components/flyout/manager/layout_mode.ts‎

Lines changed: 36 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Side Public License, v 1.
77
*/
88

9-
import React, { useEffect, useState } from 'react';
9+
import { useCallback, useEffect, useMemo, useState } from 'react';
1010
import { useEuiTheme } from '../../../services';
1111
import { setLayoutMode } from './actions';
1212
import {
@@ -37,18 +37,23 @@ export const useApplyFlyoutLayoutMode = () => {
3737

3838
const parentWidth = useFlyoutWidth(parentFlyoutId);
3939
const childWidth = useFlyoutWidth(childFlyoutId);
40+
const hasFlyouts = Boolean(parentFlyoutId);
4041

4142
const [windowWidth, setWindowWidth] = useState(
4243
typeof window !== 'undefined' ? window.innerWidth : Infinity
4344
);
4445

45-
const setMode = React.useCallback(
46+
// Extract specific context values
47+
const dispatch = context?.dispatch;
48+
const currentLayoutMode = context?.state?.layoutMode;
49+
50+
const setMode = useCallback(
4651
(layoutMode: EuiFlyoutLayoutMode) => {
47-
if (context?.dispatch && layoutMode !== context.state.layoutMode) {
48-
context.dispatch(setLayoutMode(layoutMode));
52+
if (dispatch) {
53+
dispatch(setLayoutMode(layoutMode));
4954
}
5055
},
51-
[context]
56+
[dispatch]
5257
);
5358

5459
useEffect(() => {
@@ -75,13 +80,13 @@ export const useApplyFlyoutLayoutMode = () => {
7580
};
7681
}, []);
7782

78-
useEffect(() => {
79-
if (!context) {
80-
return;
83+
// Calculate the desired layout mode
84+
const desiredLayoutMode = useMemo(() => {
85+
// Skip calculation if no flyouts open
86+
if (!hasFlyouts) {
87+
return null;
8188
}
8289

83-
const currentLayoutMode = context.state.layoutMode;
84-
8590
// Thresholds to prevent thrashing near the breakpoint.
8691
const THRESHOLD_TO_SIDE_BY_SIDE = 85;
8792
const THRESHOLD_TO_STACKED = 95;
@@ -92,16 +97,11 @@ export const useApplyFlyoutLayoutMode = () => {
9297
// `composeFlyoutSizing` in `flyout.styles.ts` multiplied
9398
// by 2 (open flyouts side-by-side).
9499
if (windowWidth < Math.round(euiTheme.breakpoint.s * 1.4)) {
95-
if (currentLayoutMode !== LAYOUT_MODE_STACKED) {
96-
setMode(LAYOUT_MODE_STACKED);
97-
}
98-
return;
100+
return LAYOUT_MODE_STACKED;
99101
}
100102

101103
if (!childFlyoutId) {
102-
if (currentLayoutMode !== LAYOUT_MODE_SIDE_BY_SIDE)
103-
setMode(LAYOUT_MODE_SIDE_BY_SIDE);
104-
return;
104+
return LAYOUT_MODE_SIDE_BY_SIDE;
105105
}
106106

107107
let parentWidthValue = parentWidth;
@@ -116,54 +116,49 @@ export const useApplyFlyoutLayoutMode = () => {
116116
}
117117

118118
if (!parentWidthValue || !childWidthValue) {
119-
if (currentLayoutMode !== LAYOUT_MODE_SIDE_BY_SIDE)
120-
setMode(LAYOUT_MODE_SIDE_BY_SIDE);
121-
return;
119+
return LAYOUT_MODE_SIDE_BY_SIDE;
122120
}
123121

124122
const combinedWidth = parentWidthValue + childWidthValue;
125123
const combinedWidthPercentage = (combinedWidth / windowWidth) * 100;
126-
let newLayoutMode: EuiFlyoutLayoutMode;
127124

128125
// Handle fill size flyouts: keep layout as side-by-side when fill flyout is present
129126
// This allows fill flyouts to dynamically calculate their width based on the other in the pair
130127
if (parentFlyout?.size === 'fill' || childFlyout?.size === 'fill') {
131128
// For fill flyouts, we want to maintain side-by-side layout to enable dynamic width calculation
132129
// Only stack if the viewport is too small (below the small breakpoint)
133130
if (windowWidth >= Math.round(euiTheme.breakpoint.s * 1.4)) {
134-
if (currentLayoutMode !== LAYOUT_MODE_SIDE_BY_SIDE) {
135-
setMode(LAYOUT_MODE_SIDE_BY_SIDE);
136-
}
137-
return;
131+
return LAYOUT_MODE_SIDE_BY_SIDE;
138132
}
139133
}
140134

141135
if (currentLayoutMode === LAYOUT_MODE_STACKED) {
142-
newLayoutMode =
143-
combinedWidthPercentage <= THRESHOLD_TO_SIDE_BY_SIDE
144-
? LAYOUT_MODE_SIDE_BY_SIDE
145-
: LAYOUT_MODE_STACKED;
136+
return combinedWidthPercentage <= THRESHOLD_TO_SIDE_BY_SIDE
137+
? LAYOUT_MODE_SIDE_BY_SIDE
138+
: LAYOUT_MODE_STACKED;
146139
} else {
147-
newLayoutMode =
148-
combinedWidthPercentage >= THRESHOLD_TO_STACKED
149-
? LAYOUT_MODE_STACKED
150-
: LAYOUT_MODE_SIDE_BY_SIDE;
151-
}
152-
153-
if (currentLayoutMode !== newLayoutMode) {
154-
setMode(newLayoutMode);
140+
return combinedWidthPercentage >= THRESHOLD_TO_STACKED
141+
? LAYOUT_MODE_STACKED
142+
: LAYOUT_MODE_SIDE_BY_SIDE;
155143
}
156144
}, [
145+
hasFlyouts,
157146
windowWidth,
158-
context,
147+
euiTheme,
148+
childFlyoutId,
159149
parentWidth,
160-
setMode,
161150
childWidth,
162-
childFlyoutId,
163151
parentFlyout?.size,
164152
childFlyout?.size,
165-
euiTheme,
153+
currentLayoutMode,
166154
]);
155+
156+
// Apply the desired layout mode
157+
useEffect(() => {
158+
if (desiredLayoutMode && currentLayoutMode !== desiredLayoutMode) {
159+
setMode(desiredLayoutMode);
160+
}
161+
}, [desiredLayoutMode, currentLayoutMode, setMode]);
167162
};
168163

169164
/** Convert a flyout `size` value to a pixel width using theme breakpoints. */

0 commit comments

Comments
 (0)