Skip to content

Commit fa1769c

Browse files
authored
fix(frontend): prevent memory leak from uncleaned storage listener in useFeatureFlag (#1295)
## Root cause `useFeatureFlag` called `window.addEventListener('storage', evaluate)` inside `useIsomorphicLayoutEffect` but **never returned a cleanup function**. Each time `flag` or `scrollTargetId` changed, React re-ran the effect — adding a new listener without removing the previous one. After `n` re-renders, `n` orphaned listener references accumulated in `window._eventListeners`, each retaining the `evaluate` closure (and with it `flag`, `scrollTargetId`, `setIsEnabled`, etc.) in memory. ## Fix (`src/hooks/useFeatureFlag.ts`) 1. Extract the body as a named `evaluate()` function so the **same reference** is passed to both `addEventListener` and `removeEventListener` (required for correct deregistration). 2. Return `() => window.removeEventListener('storage', evaluate)` from the effect so React cleans up on unmount **and** before every re-run triggered by a dep change. ```diff - const newEnabled = getFeatureFlag(flag); - setIsEnabled(newEnabled); - … + function evaluate() { + const newEnabled = getFeatureFlag(flag); + setIsEnabled(newEnabled); + … + } + + evaluate(); + + if (typeof window !== 'undefined') { + window.addEventListener('storage', evaluate); + return () => window.removeEventListener('storage', evaluate); + } }, [flag, scrollTargetId]); ``` The `storage` listener also makes flag state reactive: toggling a flag via devtools or another tab now updates the component without a page reload. ## Regression tests (`src/hooks/__tests__/useFeatureFlag.test.ts`) - **`removes storage listener on unmount so it does not leak`** — spies on `window.addEventListener`/`removeEventListener` and asserts the exact handler registered is also removed when the hook unmounts. - **`re-evaluates the flag when a storage event fires`** — dispatches a `StorageEvent` after changing the mock return value and asserts the hook state updates. Closes #1220
1 parent adaa4fd commit fa1769c

2 files changed

Lines changed: 64 additions & 9 deletions

File tree

Dechat/dex_with_fiat_frontend/src/hooks/__tests__/useFeatureFlag.test.ts

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { renderHook } from '@testing-library/react';
1+
import { renderHook, act } from '@testing-library/react';
2+
import { vi, describe, it, expect, afterEach } from 'vitest';
23
import { useFeatureFlag } from '../useFeatureFlag';
34
import * as featureFlags from '@/lib/featureFlags';
45

@@ -42,4 +43,45 @@ describe('useFeatureFlag', () => {
4243

4344
expect(result.current).toBe(true);
4445
});
46+
47+
// ── memory-leak regression (#1220) ─────────────────────────────────────────
48+
49+
it('regression: removes storage listener on unmount so it does not leak', () => {
50+
const addSpy = vi.spyOn(window, 'addEventListener');
51+
const removeSpy = vi.spyOn(window, 'removeEventListener');
52+
53+
vi.spyOn(featureFlags, 'getFeatureFlag').mockReturnValue(false);
54+
55+
const { unmount } = renderHook(() => useFeatureFlag('enableHaptics'));
56+
57+
// A storage listener must have been registered.
58+
const storageListeners = addSpy.mock.calls.filter(([type]) => type === 'storage');
59+
expect(storageListeners.length).toBeGreaterThan(0);
60+
61+
const registeredHandler = storageListeners[0][1];
62+
63+
unmount();
64+
65+
// On unmount the same handler must be removed — no leak.
66+
const removedStorageListeners = removeSpy.mock.calls.filter(
67+
([type, fn]) => type === 'storage' && fn === registeredHandler,
68+
);
69+
expect(removedStorageListeners.length).toBeGreaterThan(0);
70+
});
71+
72+
it('re-evaluates the flag when a storage event fires', () => {
73+
let flagEnabled = false;
74+
vi.spyOn(featureFlags, 'getFeatureFlag').mockImplementation(() => flagEnabled);
75+
76+
const { result } = renderHook(() => useFeatureFlag('enableHaptics'));
77+
expect(result.current).toBe(false);
78+
79+
// Simulate a storage change that flips the flag.
80+
flagEnabled = true;
81+
act(() => {
82+
window.dispatchEvent(new StorageEvent('storage'));
83+
});
84+
85+
expect(result.current).toBe(true);
86+
});
4587
});

Dechat/dex_with_fiat_frontend/src/hooks/useFeatureFlag.ts

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,30 @@ export function useFeatureFlag(flag: FeatureFlag, scrollTargetId?: string) {
5757
return;
5858
}
5959

60-
const newEnabled = getFeatureFlag(flag);
61-
setIsEnabled(newEnabled);
62-
trackFeatureFlag(flag, newEnabled);
60+
function evaluate() {
61+
const newEnabled = getFeatureFlag(flag);
62+
setIsEnabled(newEnabled);
63+
trackFeatureFlag(flag, newEnabled);
6364

64-
// Auto-scroll behavior: if flag becomes enabled and scrollTargetId is provided, scroll to it
65-
if (newEnabled && scrollTargetId && typeof window !== 'undefined') {
66-
const element = document.getElementById(scrollTargetId);
67-
if (element) {
68-
element.scrollIntoView({ behavior: 'smooth', block: 'center' });
65+
// Auto-scroll behavior: if flag becomes enabled and scrollTargetId is provided, scroll to it
66+
if (newEnabled && scrollTargetId && typeof window !== 'undefined') {
67+
const element = document.getElementById(scrollTargetId);
68+
if (element) {
69+
element.scrollIntoView({ behavior: 'smooth', block: 'center' });
70+
}
6971
}
7072
}
73+
74+
evaluate();
75+
76+
// Re-evaluate when another tab or a dev-tools script modifies localStorage so
77+
// that flag overrides applied at runtime are picked up without a page reload.
78+
// The listener MUST be removed on cleanup — failing to do so leaks it on every
79+
// re-render that changes `flag` or `scrollTargetId` (#1220).
80+
if (typeof window !== 'undefined') {
81+
window.addEventListener('storage', evaluate);
82+
return () => window.removeEventListener('storage', evaluate);
83+
}
7184
}, [flag, scrollTargetId]);
7285

7386
return isEnabled;

0 commit comments

Comments
 (0)