From 6a8cebc1f39c4adfd8309ae339820d450258ac79 Mon Sep 17 00:00:00 2001 From: elcabasa Date: Thu, 23 Jul 2026 17:16:58 +0100 Subject: [PATCH 1/2] test(sidebar): add event listener lifecycle cleanup tests Verify that all window and matchMedia listeners registered by SidebarProvider and useIsMobile are properly cleaned up on unmount using identical callback references. --- src/shared/components/ui/sidebar.test.tsx | 192 ++++++++++++++++++++++ 1 file changed, 192 insertions(+) create mode 100644 src/shared/components/ui/sidebar.test.tsx diff --git a/src/shared/components/ui/sidebar.test.tsx b/src/shared/components/ui/sidebar.test.tsx new file mode 100644 index 0000000..c9658f8 --- /dev/null +++ b/src/shared/components/ui/sidebar.test.tsx @@ -0,0 +1,192 @@ +// @vitest-environment jsdom +import { describe, it, expect, vi } from 'vitest' +import { render } from '@testing-library/react' +import { act } from 'react' +import { + SidebarProvider, + Sidebar, + SidebarContent, + SidebarGroup, + SidebarGroupContent, + SidebarMenu, + SidebarMenuItem, + SidebarMenuButton, +} from './sidebar' + +type Listener = (event: Event) => void + +function SidebarHarness() { + return ( + + + + + + + + Item + + + + + + + + ) +} + +describe('Sidebar event listener lifecycle', () => { + const origAdd = window.addEventListener.bind(window) + const origRemove = window.removeEventListener.bind(window) + + it('registers and removes the same number of listeners across repeated mount/unmount cycles', () => { + let addCount = 0 + let removeCount = 0 + + window.addEventListener = vi.fn((type: string, listener: Listener) => { + addCount++ + origAdd(type, listener) + }) as typeof window.addEventListener + window.removeEventListener = vi.fn((type: string, listener: Listener) => { + removeCount++ + origRemove(type, listener) + }) as typeof window.removeEventListener + + try { + for (let i = 0; i < 5; i++) { + const result = render() + act(() => result.unmount()) + } + expect(addCount).toBe(removeCount) + expect(addCount).toBeGreaterThan(0) + } finally { + window.addEventListener = origAdd + window.removeEventListener = origRemove + } + }) + + it('adds and removes the exact same callback reference for window keydown', () => { + const addedCallbacks: Listener[] = [] + const removedCallbacks: Listener[] = [] + + window.addEventListener = vi.fn((type: string, listener: Listener) => { + if (type === 'keydown') addedCallbacks.push(listener) + origAdd(type, listener) + }) as typeof window.addEventListener + window.removeEventListener = vi.fn((type: string, listener: Listener) => { + if (type === 'keydown') removedCallbacks.push(listener) + origRemove(type, listener) + }) as typeof window.removeEventListener + + try { + const result = render() + act(() => result.unmount()) + + expect(addedCallbacks.length).toBeGreaterThanOrEqual(1) + expect(removedCallbacks.length).toBeGreaterThanOrEqual(1) + + for (const fn of addedCallbacks) { + expect(removedCallbacks).toContain(fn) + } + } finally { + window.addEventListener = origAdd + window.removeEventListener = origRemove + } + }) + + it('does not fire stale keyboard handler after unmount', () => { + const registered = new Set() + + window.addEventListener = vi.fn((type: string, listener: Listener) => { + if (type === 'keydown') registered.add(listener) + origAdd(type, listener) + }) as typeof window.addEventListener + window.removeEventListener = vi.fn((type: string, listener: Listener) => { + if (type === 'keydown') registered.delete(listener) + origRemove(type, listener) + }) as typeof window.removeEventListener + + try { + const result = render() + const handlersBeforeUnmount = new Set(registered) + + act(() => result.unmount()) + + for (const handler of handlersBeforeUnmount) { + expect(registered.has(handler)).toBe(false) + } + } finally { + window.addEventListener = origAdd + window.removeEventListener = origRemove + } + }) + + it('uses the same function identity for add and remove on window', () => { + const pairs = new Map() + + window.addEventListener = vi.fn((type: string, listener: Listener) => { + if (!pairs.has(type)) pairs.set(type, { added: [], removed: [] }) + pairs.get(type)!.added.push(listener) + origAdd(type, listener) + }) as typeof window.addEventListener + window.removeEventListener = vi.fn((type: string, listener: Listener) => { + if (!pairs.has(type)) pairs.set(type, { added: [], removed: [] }) + pairs.get(type)!.removed.push(listener) + origRemove(type, listener) + }) as typeof window.removeEventListener + + try { + const result = render() + act(() => result.unmount()) + + for (const [, { added, removed }] of pairs) { + for (const fn of added) { + expect(removed).toContain(fn) + } + } + } finally { + window.addEventListener = origAdd + window.removeEventListener = origRemove + } + }) +}) + +describe('useIsMobile matchMedia listener lifecycle', () => { + it('cleans up matchMedia change listener on unmount', () => { + const added: Listener[] = [] + const removed: Listener[] = [] + + const mockMql = { + matches: false, + media: '(max-width: 767px)', + onchange: null, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn((_type: string, handler: Listener) => { + added.push(handler) + }), + removeEventListener: vi.fn((_type: string, handler: Listener) => { + const idx = added.indexOf(handler) + if (idx >= 0) removed.push(handler) + }), + dispatchEvent: vi.fn(), + } + + const originalMatchMedia = window.matchMedia + window.matchMedia = vi.fn().mockReturnValue(mockMql) + + try { + const result = render() + + expect(mockMql.addEventListener).toHaveBeenCalledWith('change', expect.any(Function)) + const addedCount = added.length + + act(() => result.unmount()) + + expect(mockMql.removeEventListener).toHaveBeenCalledWith('change', expect.any(Function)) + expect(removed.length).toBe(addedCount) + } finally { + window.matchMedia = originalMatchMedia + } + }) +}) From b9cdc6984a49ff4cc8eaf39a3e3ab675f0e34b25 Mon Sep 17 00:00:00 2001 From: elcabasa Date: Thu, 23 Jul 2026 17:28:26 +0100 Subject: [PATCH 2/2] fix: remove duplicate retry declaration in useOptimisticData Removes the second retry callback (which referenced undefined MAX_RETRIES) that shadowed the correct implementation above. --- src/shared/hooks/useOptimisticData.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/shared/hooks/useOptimisticData.ts b/src/shared/hooks/useOptimisticData.ts index 03aec6b..6635423 100644 --- a/src/shared/hooks/useOptimisticData.ts +++ b/src/shared/hooks/useOptimisticData.ts @@ -256,13 +256,6 @@ export function useOptimisticData( }, delay) }, [fetchData, maxRetries, baseDelay, clearBackoffTimer]) - const retry = useCallback(() => { - if (retryCount < MAX_RETRIES && lastFetchFnRef.current) { - setRetryCount((c) => c + 1); - fetchData(lastFetchFnRef.current, true); - } - }, [retryCount, fetchData]); - const clearCache = useCallback(() => { cacheRef.current.clear() }, [])