Skip to content

Commit 0e2497c

Browse files
kotAPIkotAPI
authored
feat(perf): overlay listener cleanup audits (rad-ui#1932)
1 parent 0b75501 commit 0e2497c

2 files changed

Lines changed: 337 additions & 1 deletion

File tree

‎src/components/ui/HoverCard/fragments/HoverCardContent.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,15 @@ const HoverCardContent = forwardRef<HoverCardContentElement, HoverCardContentPro
2222
} = useContext(HoverCardContext);
2323

2424
useEffect(() => {
25+
if (!isOpen) return;
26+
2527
const handleScroll = () => closeWithoutDelay();
2628
window.addEventListener('scroll', handleScroll);
2729

2830
return () => {
2931
window.removeEventListener('scroll', handleScroll);
3032
};
31-
}, [closeWithoutDelay]);
33+
}, [closeWithoutDelay, isOpen]);
3234

3335
const mergedRef = Floater.useMergeRefs([floatingRefs.setFloating, ref]);
3436
const dataAttributes = createDataAttributes('hover-card', { size });

‎src/components/ui/tests/Overlay.cleanup.test.tsx‎

Lines changed: 334 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,334 @@
1+
import React from 'react';
2+
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
3+
import Dialog from '~/components/ui/Dialog/Dialog';
4+
import HoverCard from '~/components/ui/HoverCard/HoverCard';
5+
import Popover from '~/components/ui/Popover/Popover';
6+
import Tooltip from '~/components/ui/Tooltip/Tooltip';
7+
8+
type ListenerTarget = Window | Document;
9+
type ListenerMap = Record<'window' | 'document', string[]>;
10+
11+
const TARGETS: Array<{ name: keyof ListenerMap; target: ListenerTarget }> = [
12+
{ name: 'window', target: window },
13+
{ name: 'document', target: document }
14+
];
15+
const TRACKED_EVENT_TYPES = new Set([
16+
'click',
17+
'focusin',
18+
'focusout',
19+
'keydown',
20+
'mousedown',
21+
'mouseup',
22+
'pointerdown',
23+
'pointermove',
24+
'pointerup',
25+
'resize',
26+
'scroll',
27+
'touchend',
28+
'touchmove',
29+
'touchstart'
30+
]);
31+
32+
function createGlobalListenerTracker() {
33+
const originals = new Map<ListenerTarget, {
34+
addEventListener: typeof window.addEventListener;
35+
removeEventListener: typeof window.removeEventListener;
36+
}>();
37+
const active = {
38+
window: new Map<string, number>(),
39+
document: new Map<string, number>()
40+
} satisfies Record<keyof ListenerMap, Map<string, number>>;
41+
const listenerIds = new WeakMap<EventListenerOrEventListenerObject, number>();
42+
let nextListenerId = 0;
43+
44+
const getListenerId = (listener: EventListenerOrEventListenerObject | null) => {
45+
if (!listener) {
46+
return 'null';
47+
}
48+
49+
const existingId = listenerIds.get(listener);
50+
if (existingId != null) {
51+
return String(existingId);
52+
}
53+
54+
nextListenerId += 1;
55+
listenerIds.set(listener, nextListenerId);
56+
return String(nextListenerId);
57+
};
58+
59+
const normalizeCapture = (options?: boolean | AddEventListenerOptions | EventListenerOptions) =>
60+
typeof options === 'boolean' ? options : Boolean(options?.capture);
61+
62+
const record = (map: Map<string, number>, type: string, listener: EventListenerOrEventListenerObject | null, options?: boolean | AddEventListenerOptions | EventListenerOptions) => {
63+
if (!TRACKED_EVENT_TYPES.has(type)) {
64+
return '';
65+
}
66+
67+
const key = `${type}:${normalizeCapture(options)}:${getListenerId(listener)}`;
68+
map.set(key, (map.get(key) ?? 0) + 1);
69+
return key;
70+
};
71+
72+
const removeRecord = (map: Map<string, number>, type: string, listener: EventListenerOrEventListenerObject | null, options?: boolean | AddEventListenerOptions | EventListenerOptions) => {
73+
if (!TRACKED_EVENT_TYPES.has(type)) {
74+
return;
75+
}
76+
77+
const key = `${type}:${normalizeCapture(options)}:${getListenerId(listener)}`;
78+
const current = map.get(key) ?? 0;
79+
80+
if (current <= 1) {
81+
map.delete(key);
82+
return;
83+
}
84+
85+
map.set(key, current - 1);
86+
};
87+
88+
for (const { name, target } of TARGETS) {
89+
originals.set(target, {
90+
addEventListener: target.addEventListener,
91+
removeEventListener: target.removeEventListener
92+
});
93+
94+
target.addEventListener = ((type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions) => {
95+
record(active[name], type, listener, options);
96+
return originals.get(target)!.addEventListener.call(target, type, listener, options);
97+
}) as typeof target.addEventListener;
98+
99+
target.removeEventListener = ((type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions) => {
100+
removeRecord(active[name], type, listener, options);
101+
return originals.get(target)!.removeEventListener.call(target, type, listener, options);
102+
}) as typeof target.removeEventListener;
103+
}
104+
105+
const snapshot = (): ListenerMap => ({
106+
window: Array.from(active.window.entries()).flatMap(([key, count]) => Array(count).fill(key)).sort(),
107+
document: Array.from(active.document.entries()).flatMap(([key, count]) => Array(count).fill(key)).sort()
108+
});
109+
110+
const totalActive = () => snapshot().window.length + snapshot().document.length;
111+
112+
const restore = () => {
113+
for (const { target } of TARGETS) {
114+
const original = originals.get(target);
115+
if (!original) continue;
116+
117+
target.addEventListener = original.addEventListener;
118+
target.removeEventListener = original.removeEventListener;
119+
}
120+
};
121+
122+
return {
123+
snapshot,
124+
totalActive,
125+
restore
126+
};
127+
}
128+
129+
describe('Overlay listener cleanup', () => {
130+
test('Dialog cleans up global listeners after close and unmount while open', async() => {
131+
const tracker = createGlobalListenerTracker();
132+
133+
try {
134+
const { unmount } = render(
135+
<Dialog.Root>
136+
<Dialog.Trigger>Open dialog</Dialog.Trigger>
137+
<Dialog.Overlay />
138+
<Dialog.Content>
139+
<Dialog.Title>Dialog title</Dialog.Title>
140+
<Dialog.Close>Close dialog</Dialog.Close>
141+
</Dialog.Content>
142+
</Dialog.Root>
143+
);
144+
145+
const baseline = tracker.snapshot();
146+
const baselineTotal = tracker.totalActive();
147+
148+
fireEvent.click(screen.getByRole('button', { name: 'Open dialog' }));
149+
await screen.findByRole('dialog');
150+
expect(tracker.totalActive()).toBeGreaterThan(baselineTotal);
151+
152+
fireEvent.click(screen.getByRole('button', { name: 'Close dialog' }));
153+
await waitFor(() => {
154+
expect(screen.queryByRole('dialog')).toBeNull();
155+
expect(tracker.snapshot()).toEqual(baseline);
156+
});
157+
158+
fireEvent.click(screen.getByRole('button', { name: 'Open dialog' }));
159+
await screen.findByRole('dialog');
160+
expect(tracker.totalActive()).toBeGreaterThan(baselineTotal);
161+
162+
unmount();
163+
await waitFor(() => {
164+
expect(tracker.snapshot()).toEqual(baseline);
165+
});
166+
} finally {
167+
tracker.restore();
168+
}
169+
});
170+
171+
test('Popover does not accumulate listeners across repeated open and close cycles', async() => {
172+
const tracker = createGlobalListenerTracker();
173+
174+
try {
175+
render(
176+
<div>
177+
<button>Outside</button>
178+
<Popover.Root>
179+
<Popover.Trigger>Open popover</Popover.Trigger>
180+
<Popover.Content>Popover body</Popover.Content>
181+
</Popover.Root>
182+
</div>
183+
);
184+
185+
const baseline = tracker.snapshot();
186+
const baselineTotal = tracker.totalActive();
187+
188+
fireEvent.click(screen.getByRole('button', { name: 'Open popover' }));
189+
await screen.findByRole('dialog');
190+
const firstOpenCount = tracker.totalActive();
191+
expect(firstOpenCount).toBeGreaterThan(baselineTotal);
192+
193+
fireEvent.pointerDown(screen.getByRole('button', { name: 'Outside' }));
194+
await waitFor(() => {
195+
expect(screen.queryByRole('dialog')).toBeNull();
196+
expect(tracker.snapshot()).toEqual(baseline);
197+
});
198+
199+
fireEvent.click(screen.getByRole('button', { name: 'Open popover' }));
200+
await screen.findByRole('dialog');
201+
expect(tracker.totalActive()).toBe(firstOpenCount);
202+
203+
fireEvent.pointerDown(screen.getByRole('button', { name: 'Outside' }));
204+
await waitFor(() => {
205+
expect(screen.queryByRole('dialog')).toBeNull();
206+
expect(tracker.snapshot()).toEqual(baseline);
207+
});
208+
} finally {
209+
tracker.restore();
210+
}
211+
});
212+
213+
test('Nested popovers clean up listeners as inner and outer overlays close', async() => {
214+
const tracker = createGlobalListenerTracker();
215+
216+
try {
217+
render(
218+
<div>
219+
<button>Outside</button>
220+
<Popover.Root>
221+
<Popover.Trigger>Open outer</Popover.Trigger>
222+
<Popover.Content>
223+
<span>Outer body</span>
224+
<Popover.Root>
225+
<Popover.Trigger>Open inner</Popover.Trigger>
226+
<Popover.Content>
227+
<span>Inner body</span>
228+
<Popover.Close>Close inner</Popover.Close>
229+
</Popover.Content>
230+
</Popover.Root>
231+
</Popover.Content>
232+
</Popover.Root>
233+
</div>
234+
);
235+
236+
const baseline = tracker.snapshot();
237+
238+
fireEvent.click(screen.getByRole('button', { name: 'Open outer' }));
239+
await screen.findByText('Outer body');
240+
fireEvent.click(screen.getByRole('button', { name: 'Open inner' }));
241+
await screen.findByText('Inner body');
242+
expect(tracker.totalActive()).toBeGreaterThan(0);
243+
244+
fireEvent.click(screen.getByRole('button', { name: 'Close inner' }));
245+
await waitFor(() => {
246+
expect(screen.queryByText('Inner body')).toBeNull();
247+
expect(screen.getByText('Outer body')).toBeInTheDocument();
248+
});
249+
250+
fireEvent.pointerDown(screen.getByRole('button', { name: 'Outside' }));
251+
await waitFor(() => {
252+
expect(screen.queryByText('Outer body')).toBeNull();
253+
expect(tracker.snapshot()).toEqual(baseline);
254+
});
255+
} finally {
256+
tracker.restore();
257+
}
258+
});
259+
260+
test('Tooltip only keeps global listeners during the active interaction lifecycle', async() => {
261+
const tracker = createGlobalListenerTracker();
262+
263+
try {
264+
const { unmount } = render(
265+
<Tooltip.Root>
266+
<Tooltip.Trigger>Tooltip trigger</Tooltip.Trigger>
267+
<Tooltip.Content>Tooltip body</Tooltip.Content>
268+
</Tooltip.Root>
269+
);
270+
271+
const baseline = tracker.snapshot();
272+
const baselineTotal = tracker.totalActive();
273+
274+
fireEvent.mouseEnter(screen.getByText('Tooltip trigger'));
275+
await screen.findByRole('tooltip');
276+
277+
fireEvent.mouseLeave(screen.getByText('Tooltip trigger'));
278+
await waitFor(() => {
279+
expect(screen.queryByRole('tooltip')).toBeNull();
280+
expect(tracker.snapshot()).toEqual(baseline);
281+
});
282+
283+
fireEvent.mouseEnter(screen.getByText('Tooltip trigger'));
284+
await screen.findByRole('tooltip');
285+
286+
unmount();
287+
await waitFor(() => {
288+
expect(tracker.snapshot()).toEqual(baseline);
289+
});
290+
} finally {
291+
tracker.restore();
292+
}
293+
});
294+
295+
test('HoverCard ties scroll and dismissal listeners to open state and cleans them on unmount', async() => {
296+
const tracker = createGlobalListenerTracker();
297+
298+
try {
299+
const beforeRender = tracker.snapshot();
300+
const { unmount } = render(
301+
<HoverCard.Root openDelay={0} closeDelay={0}>
302+
<HoverCard.Trigger>Hover trigger</HoverCard.Trigger>
303+
<HoverCard.Content>Hover body</HoverCard.Content>
304+
</HoverCard.Root>
305+
);
306+
307+
const baseline = tracker.snapshot();
308+
const baselineTotal = tracker.totalActive();
309+
expect(baseline).toEqual(beforeRender);
310+
311+
fireEvent.mouseEnter(screen.getByText('Hover trigger'));
312+
await screen.findByRole('dialog');
313+
const activeCount = tracker.totalActive();
314+
expect(activeCount).toBeGreaterThan(baselineTotal);
315+
316+
fireEvent.mouseLeave(screen.getByText('Hover trigger'));
317+
await waitFor(() => {
318+
expect(screen.queryByRole('dialog')).toBeNull();
319+
expect(tracker.snapshot()).toEqual(baseline);
320+
});
321+
322+
fireEvent.mouseEnter(screen.getByText('Hover trigger'));
323+
await screen.findByRole('dialog');
324+
expect(tracker.totalActive()).toBe(activeCount);
325+
326+
unmount();
327+
await waitFor(() => {
328+
expect(tracker.snapshot()).toEqual(baseline);
329+
});
330+
} finally {
331+
tracker.restore();
332+
}
333+
});
334+
});

0 commit comments

Comments
 (0)