useEffect Cleanup Never Runs: The Subscribed Listener Leak

A scroll-tracking hook adding a `scroll` listener in useEffect. No cleanup. Each mount stacks a listener; unmount leaves the old one behind. After 5 mount/unmount cycles, the callback fires 5× per scroll event.

The vulnerable diff

// hooks/useScroll.ts
export function useScroll(onScroll: (y: number) => void) {
  useEffect(() => {
    // BUG: no cleanup — listener accumulates on every mount
    window.addEventListener("scroll", () => onScroll(window.scrollY));
  }, []);
}

What is wrong

React effects are mount + cleanup pairs by design. Every subscription you add inside the effect must be matched by a removal in the cleanup function. Without it, mount/unmount cycles (route navigation, conditional render, hot reload) stack subscriptions. After N cycles the callback runs N times per event. Memory grows because each handler is a closure holding a reference to props from its mount cycle.

The attack

Reproducer: log in the handler, navigate between pages 5 times:

# After 5 navigations:
[1] scroll y=200
[2] scroll y=200
[3] scroll y=200
[4] scroll y=200
[5] scroll y=200  // 5 callbacks per scroll event

If the callback does any meaningful work — analytics ping, layout calc — the cost grows linearly.

Mesrai's review comment

mesraipilot · Bot · reviewed 1 min ago

[mesrai] [code-review] [Performance] [React-Leak] [medium]

`window.addEventListener` inside useEffect without paired remove.
Listener leaks per mount/unmount cycle.

Always return a cleanup function:

  useEffect(() => {
    const handler = () => onScroll(window.scrollY);
    window.addEventListener("scroll", handler);
    return () => window.removeEventListener("scroll", handler);
  }, [onScroll]);

Named handler so the cleanup can reference the same function (you
cannot remove an inline arrow). Include `onScroll` in deps if the
callback can change.

The fix

// hooks/useScroll.ts — fixed
export function useScroll(onScroll: (y: number) => void) {
  useEffect(() => {
    const handler = () => onScroll(window.scrollY);
    window.addEventListener("scroll", handler);
    return () => window.removeEventListener("scroll", handler);
  }, [onScroll]);
}

Named handler, paired add/remove, dep on the callback so React re-subscribes when the callback identity changes. Standard React subscribe/cleanup pattern.

Why human review missed it

Cleanup is the canonical React effect mistake — the mount side is obvious, the cleanup side is the discipline. ESLint's `react-hooks/exhaustive-deps` partially helps. Mesrai catches every `addEventListener`, `setInterval`, `subscribe()` call in a useEffect without a paired cleanup.

Related rules + further reading

Mesrai rule pack: language/react-effect-cleanup — flags subscribe-shape calls in useEffect without cleanup.

React docs: Synchronizing with Effects § You Might Not Need an Effect.

Most common React memory-leak class.

Takeaway

Every subscription has a cleanup. Mount adds, cleanup removes. Mesrai catches every gap.