React useEffect Mistakes I Keep Reviewing in PRs

Effect vs event: the question I ask first

If something happens because the user clicked, put it in the click handler. If something synchronizes React with an external system such as document title, WebSocket, or a non-React widget, use an effect. I still see useEffect that saves draft form on every keystroke and hammers the API — that belongs behind a debounce in an event or a dedicated autosave hook with clear intent.

React docs pushed this mental model hard, and our bug rate dropped once we enforced it in review by asking what external system this is syncing.

Concrete number: on one form, autosave-via-effect fired about 40 requests per minute of typing. Moving to a debounced blur or interval save cut that to about 4 and removed a rate-limit incident on the draft endpoint.

The infinite loop starter pack

Classic: effect sets state that is also in the dependency array, without a guard. Or depending on an object or array literal created each render. Or depending on a function prop that the parent recreates every render.

I ask authors to log render counts for a few minutes when they suspect a loop. Seeing 200-plus renders per second in Strict Mode is a strong signal before the tab locks up.

// Loop: new options object every render → effect → setState → render → ...
function Chart({ data }) {
  const [series, setSeries] = useState([]);
  const options = { animate: true }; // new reference each time

  useEffect(() => {
    setSeries(transform(data, options));
  }, [data, options]);

  return <Canvas series={series} />;
}

// Fix: depend on primitives / stable values
useEffect(() => {
  setSeries(transform(data, { animate: true }));
}, [data]);

Fetch without cleanup equals race

User switches from product 1 to product 2 quickly. Request 1 finishes after request 2 and overwrites state. I have watched this show the wrong product name in QA. AbortController in cleanup is the default fix.

If you cannot abort, at least ignore stale responses with an incrementing request id or a cancelled flag. The wrong-name bug shipped once because QA clicked slowly.

useEffect(() => {
  const controller = new AbortController();
  let cancelled = false;

  async function load() {
    try {
      const res = await fetch(`/api/products/${id}`, { signal: controller.signal });
      const json = await res.json();
      if (!cancelled) setProduct(json);
    } catch (err) {
      if (err.name !== 'AbortError') setError(err);
    }
  }

  load();
  return () => {
    cancelled = true;
    controller.abort();
  };
}, [id]);

Strict Mode double mount is not your enemy

In React 18-plus development, effects mount, clean up, and mount again to surface missing cleanup. Developers fix this by removing Strict Mode. That hides bugs. If your effect opens a socket or increments an analytics counter twice in dev, add proper cleanup and idempotent setup.

We once double-charged a metering API in production after a remount path we never tested — Strict Mode would have caught the missing cleanup in dev.

Derived state that should not be state

If you can compute fullName from first and last during render, do not store it in state updated by an effect. Effects for pure derivation add an extra paint and drift bugs. Same for filtering a list: derive during render or memoize if the list is huge. We only memoized after 5k-plus rows.

Prop-to-state sync effects are another smell. Prefer fully controlled components or remount with key equals id when you truly need fresh local state per entity.

• Empty deps means once per mount — confirm that is intentional
• Omit deps only with a documented eslint-disable and a reason
• Cleanup subscriptions, timers, and in-flight fetches
• Do not put React Query or SWR fetch solely in raw useEffect if the library already owns cache
• Prefer keying a child to reset state instead of syncing props to state in effects

Subscriptions done right

Window listeners, IntersectionObserver, and media queries need subscribe in the effect and unsubscribe in cleanup. I also keep the handler stable or read latest values via a ref when the listener must stay registered once.

Forums still recommend empty deps plus eslint-disable for listeners that close over state. Prefer a ref for latest state: register once, read ref.current inside the handler. That avoids resubscribing on every render without lying about dependencies.

useEffect(() => {
  function onResize() {
    setWidth(window.innerWidth);
  }
  window.addEventListener('resize', onResize);
  onResize();
  return () => window.removeEventListener('resize', onResize);
}, []);

Timers and intervals I keep rejecting

Polling with setInterval inside an effect without clearing it on unmount or dependency change leaks timers. I have seen tabs with dozens of intervals after soft navigation in an SPA shell.

Prefer setTimeout chains when the interval should wait for the previous fetch to finish, or use a library that owns polling. Always return clearInterval or clearTimeout from the effect cleanup.

useEffect(() => {
  let timer = null;
  let stopped = false;

  async function tick() {
    await refreshInbox();
    if (!stopped) timer = setTimeout(tick, 15000);
  }

  tick();
  return () => {
    stopped = true;
    if (timer) clearTimeout(timer);
  };
}, []);

Data libraries versus hand-rolled effects

If the team already uses React Query or SWR, I reject new useEffect fetch for server state unless there is a clear gap. Those libraries handle caching, dedupe, retries, and cancellation. Re-implementing them poorly is how we got three different loading-state conventions in one app.

Effects still make sense for wiring: syncing a query result into a non-React map SDK, or writing document.title. Keep the boundary clear — server state in the library, external sync in effects. When migrating, replace one screen at a time and delete the old effect rather than leaving both paths.

Measuring before micro-optimizing effects

Not every effect that runs twice in Strict Mode is a production bug. I care when it hits the network, writes to analytics, or mutates a third-party widget. Cosmetic double logs in development are noise.

For expensive sync work after render, I check the Profiler. One optimize PR wrapped derived data in an effect to keep render pure and made the UI worse by delaying paint. We reverted and computed during render instead. Profile first, then decide whether an effect earns its keep.

What I write in the review comment

Is this syncing an external system? If not, move to the event handler. Will this race when id changes? What cleans this up under Strict Mode double invoke? Those three questions catch most of our useEffect defects before merge.

If the author cannot answer in one sentence, I ask them to delete the effect and try again. Roughly half the time the better design appears within minutes. I keep a short internal doc with before and after snippets of these patterns; linking it from the PR template cut repeat review comments by roughly half.

Key takeaways

• Use effects for external synchronization, not for ordinary user-event logic.
• Unstable object or function dependencies are a common infinite-loop source.
• Abort in-flight fetches on cleanup to prevent racey setState.
• Strict Mode double mounting highlights missing cleanup — fix cleanup, do not remove Strict Mode.
• Prefer derived values during render over prop-to-state mirroring effects.
• Always clear timers and intervals in effect cleanup.