# Effect Cleanup and Race Conditions

Use this reference only when the effect under review is asynchronous — it fetches data, opens a connection, subscribes to a stream, or sets a timer — and you need to verify the cleanup/cancellation guard is correct.

## What people get wrong

The common bad assumption is:

> "The effect updates state when the promise resolves, so it's correct."

That is incomplete. React does not cancel in-flight promises when an effect re-runs or the component unmounts. If a slow request from an earlier render resolves *after* a faster request from a later render, the stale result can overwrite the fresh one unless the effect explicitly guards against it. This is the documented root cause of "data flashes to a previous value when switching quickly."

## Officially grounded pattern

React's own docs (`you-might-not-need-an-effect.md`, `synchronizing-with-effects.md`, and the `useEffect` reference) all converge on the same shape for fetch effects: a boolean `ignore` flag set in the cleanup function, checked before every state update derived from the async result.

```js
function SearchResults({ query }) {
  const [results, setResults] = useState([]);
  const [page, setPage] = useState(1);
  useEffect(() => {
    let ignore = false;
    fetchResults(query, page).then(json => {
      if (!ignore) {
        setResults(json);
      }
    });
    return () => {
      ignore = true;
    };
  }, [query, page]);
  // ...
}
```

The same pattern applies with `async`/`await` syntax — declare `ignore` outside the async function, check it after the `await`, and return the cleanup function that flips it:

```js
useEffect(() => {
  let ignore = false;

  async function startFetching() {
    const json = await fetchTodos(userId);
    if (!ignore) {
      setTodos(json);
    }
  }

  startFetching();

  return () => {
    ignore = true;
  };
}, [userId]);
```

`AbortController` is an equally valid, and more resource-efficient, variant when the underlying request API supports cancellation (most `fetch`-based clients do) — it actually cancels the network request instead of only ignoring its result. Prefer it when the codebase already has an established `AbortController` convention; do not introduce it as a novel pattern in a codebase that consistently uses the `ignore`-flag convention without a reason tied to the specific finding (e.g., an expensive request that should genuinely be aborted, not just ignored).

## Non-negotiable design rules

1. **Every async effect that calls `setState` from a resolved promise or callback must have a guard.** No exceptions for "it's unlikely to race in practice" — the trigger sequence (fast navigation, fast re-render, flaky network) is exactly the kind of intermittent, hard-to-reproduce condition that is expensive to debug once it reaches production. Treat the absence of a guard as a finding regardless of how unlikely the reviewer judges the race to be.
2. **The guard must be checked immediately before every state update inside the async chain**, not just the first one. An effect that fetches and then makes a second dependent call must check `ignore` (or `signal.aborted`) before each `setState`, not only the first.
3. **`StrictMode` double-invocation in development is not the same bug as a production race condition.** `StrictMode` runs setup → cleanup → setup once, synchronously, specifically to verify that the cleanup function correctly undoes the setup. If an effect breaks under `StrictMode` double-invocation, that is evidence the cleanup is incomplete or missing — it is not a `StrictMode`-only artifact to work around by disabling `StrictMode`. Do not recommend removing `StrictMode` as a fix for a broken cleanup function.
4. **A `setTimeout`/`setInterval`-based effect needs `clearTimeout`/`clearInterval` in its cleanup**, and a subscription-based effect needs the corresponding unsubscribe/disconnect call — the same guard-in-cleanup principle applies beyond fetch. Treat a missing `clearInterval`/unsubscribe as the same severity class as a missing `ignore` flag: it is a resource leak and, if the interval body calls `setState`, a potential update-on-unmounted-component or stale-closure bug.

## Severity escalation

Treat a missing cancellation guard as **HIGH severity, not MEDIUM**, when the async effect's resolution triggers:

- an authenticated write, mutation, or other state-changing network call (duplicate submission risk, not just a stale read),
- a redirect, navigation, or auth-state change (wrong-user data exposure risk),
- a write to a shared/global store rather than local component state (blast radius extends beyond the component).

Otherwise (a stale read into local UI state with no side effect beyond a flicker), MEDIUM is appropriate — still a real bug, but not a data-integrity or security risk.

## Verification targets

When repo evidence is available, verify the finding against:

- the actual async function signature — does it accept or already use an `AbortSignal`? If yes and the effect ignores it, that is a stronger finding (cancellation capability exists and is unused) than the case where no cancellation mechanism exists at all.
- whether the dependency array includes every reactive value used to construct the request (query, page, userId in the examples above) — a missing request-parameter dependency is a *different* finding (stale closure, see the dependency-array reference) that often co-occurs with a missing cleanup guard in the same effect.

## When to push back

Push back if the user asks to:

- "just add a loading spinner" as the fix for a reported race condition — a spinner does not prevent stale data from overwriting fresh data; it only hides the timing, it does not fix it,
- suppress the exhaustive-deps lint warning on an async effect instead of adding the guard — the warning is frequently the signal that led to finding the missing guard in the first place,
- disable `StrictMode` to make a race condition symptom disappear in development instead of fixing the missing cleanup — this hides the bug in dev while leaving it live in production.

# Stale Closures and Dependency Arrays

Use this reference only when a dependency-array omission or a stale-closure suspicion is present — a value is read inside an effect body but is missing from (or wrongly present in) the dependency array.

## What people get wrong

The common bad assumption is:

> "The effect's dependency array is a performance knob — trim it to reduce re-runs."

That is backwards. The dependency array is not a performance setting; it is a correctness contract. Every reactive value (props, state, and anything derived from them) that the effect body reads must be listed, or the effect closes over a stale value from the render in which it was created and keeps using that stale value until something else happens to cause a re-run. This is the documented root cause of "the effect uses an old value even though the state clearly updated."

## Officially grounded pattern: distinguishing stable from reactive values

Not every value used inside an effect is "reactive" in the sense that omitting it is a bug:

- **Provably stable, safe to omit:** the `setState` function returned by `useState`, the `dispatch` function returned by `useReducer`, and `ref.current` reads performed *inside* the effect body (refs themselves are not reactive; React guarantees `set` function identity is stable across renders).
- **Reactive, must be included if read:** any prop, any state variable, any value derived from props/state (including objects and functions recreated on every render, e.g., an inline object literal or arrow function passed as a prop).
- **Non-reactive but currently forces a re-run because it's a function/object identity that changes every render:** this is the actual problem `useEffectEvent` was designed to solve (see below) — the value is logically "the latest one," not something the effect should resynchronize over.

## Non-negotiable design rules

1. **Do not classify a missing dependency as "intentional" without evidence.** A dependency array that omits a value read in the effect body is a stale-closure finding by default. It is only safe when the omitted value is one of the provably-stable categories above, or the codebase has already isolated the non-reactive read into a mechanism designed for that purpose (see rule 3). "The author probably meant to do that" is not evidence.
2. **A dependency-array lint suppression (`// eslint-disable-next-line react-hooks/exhaustive-deps`) is itself a finding**, not a signal that the array is correct. Read the suppressed line and independently verify whether the omission is safe using rule 1's criteria.
3. **When the React version supports `useEffectEvent`** (stable as of React 19.2; confirm via `package.json` and Context7 before assuming availability), it is the documented mechanism for reading the *latest* value of a prop or state variable inside an effect without making that value reactive. Example from the docs — `onMessage` needs the current `isMuted` value without forcing the connection effect to re-run every time `isMuted` toggles:

   ```js
   function ChatRoom({ roomId }) {
     const [messages, setMessages] = useState([]);
     const [isMuted, setIsMuted] = useState(false);

     const onMessage = useEffectEvent(receivedMessage => {
       setMessages(msgs => [...msgs, receivedMessage]);
       if (!isMuted) {
         playSound();
       }
     });

     useEffect(() => {
       const connection = createConnection();
       connection.connect();
       connection.on('message', (receivedMessage) => {
         onMessage(receivedMessage);
       });
       return () => connection.disconnect();
     }, [roomId]); // ✅ All dependencies declared
   }
   ```

   The Effect Event itself (`onMessage`) is **not** reactive and must be omitted from the effect's dependency array — only `roomId` remains, because `roomId` is the value that should actually trigger resynchronization (reconnecting).
4. **On React versions without `useEffectEvent`**, do not recommend it. The pre-19.2 alternatives are: accept the value as a real dependency (and accept the effect re-running when it changes, if that is actually correct behavior), or store the latest value in a `ref` updated on every render and read `ref.current` inside the effect (a manual, documented workaround with the same intent but without the ergonomics or the "must be called during render" constraint of `useEffectEvent`). State which alternative applies and why the effect's actual resynchronization need (or lack thereof) supports it.
5. **A stale closure over a `setState` value called without the updater form is a separate, related finding.** `setCount(count + 1)` inside a callback that closes over an old `count` is a stale-closure bug even outside effects; inside an effect or an effect-scheduled callback, prefer the updater form `setCount(c => c + 1)` when the new value only depends on the previous value — this sidesteps the staleness question entirely rather than requiring the value to be a correct dependency.

## Adversarial checklist

Before accepting a dependency-array omission as safe, answer:

- Is the omitted value actually read inside the effect body, or only inside a nested function that is *not* called synchronously within the effect (e.g., only referenced in a comment or dead code)? If it's genuinely unread, there is no finding.
- Is the omitted value one of the provably-stable categories (setState/dispatch function, `ref.current`)? If not, is there a `useEffectEvent` wrapper already isolating it, and does the target React version actually support `useEffectEvent`?
- If the value were included as a real dependency, would the effect's re-run behavior actually be wrong (e.g., reconnecting a socket every time an unrelated piece of state changes)? If re-running is actually correct and just looks noisy, the fix is not to omit the dependency — it may be to reconsider whether that value belongs in the same effect at all.
- Does the effect call an inline object or array literal as a dependency (e.g., `[{ id: props.id }]`)? A new object identity is created every render, so this dependency is present but still causes the effect to re-run every render — a different but related finding: the fix is to depend on the primitive fields (`props.id`) instead of the object.

## Verification targets

- Confirm the installed React major (`package.json`) before citing `useEffectEvent` as available — it is a documented, comparatively recent addition (React 19.2). Repos on 18.x or earlier need the `ref`-based workaround instead.
- Cross-reference any stale-closure finding in an async effect against `effect-cleanup-and-race-conditions.md` — a missing request-parameter dependency (e.g., `page` omitted from a fetch effect's array) is often paired with a missing cancellation guard in the same effect, and both should be reported together as related findings, not two disconnected line items.

## When to push back

Push back if the user says:

- "just disable the lint rule" as the resolution for a stale-closure finding — the rule exists specifically to catch this class of bug; disabling it removes the signal without fixing the defect,
- "the value rarely changes so it's fine to omit" — "rarely" is not "never"; the bug still exists and will surface intermittently, which is the expensive-to-debug failure mode this skill exists to catch pre-merge,
- "add `// eslint-disable` everywhere this warns" as a blanket policy — each suppression must be independently verified per the adversarial checklist above, not applied wholesale.

# Review Workflow and Findings Contract

Use this reference for the step-by-step procedure, the anti-pattern decision tree, and the required output shape. Load the other two references (`effect-cleanup-and-race-conditions.md`, `stale-closures-and-dependency-arrays.md`) only when the step below tells you to.

## Workflow

1. **Inventory.** List every `useState`, `useEffect`, `useReducer`, and `useLayoutEffect` call site in scope, with file:line. Do not skip `useLayoutEffect` — it is not interchangeable with `useEffect`; it exists specifically for layout measurement that must happen before the browser paints, so a "convert to useEffect" recommendation is wrong unless you have confirmed the effect does not read layout geometry.
2. **Classify each effect** using the decision tree below. This step decides which reference (if any) to load next.
3. **For effects classified as "synchronizing with an external system" that are async** (fetch, subscription callback, timer), load `effect-cleanup-and-race-conditions.md` and verify a cancellation guard exists.
4. **For any effect with an object, function, or derived-value dependency**, or any effect where a value used in the body is missing from the dependency array, load `stale-closures-and-dependency-arrays.md` and verify the omission is provably safe.
5. **Produce ranked findings** using the output contract below. Rank HIGH (race condition on authenticated writes, or a confirmed anti-pattern causing an infinite loop) above MEDIUM (documented anti-pattern with no loop/data-corruption risk) above LOW (style/clarity).

## Anti-pattern decision tree

For each effect, ask in order:

1. **Does this effect synchronize with something outside React** (a DOM API, a browser API like the window title, a subscription to a non-React widget, a network connection, `localStorage`)?
   - **Yes** → this is valid effect usage per `synchronizing-with-effects`. Proceed to step 2 below (async/cleanup check) if it is asynchronous. Do not flag the effect's existence as a defect.
   - **No** → continue to question 2.
2. **Does the effect only call `setState` with a value derived from props or other state, with no external system involved?**
   - **Yes, and the derived value can be computed inline during render** → this is the documented "Adjusting state on prop change in an Effect" or "Adjusting some state when a prop changes" anti-pattern. Fix: compute the value during render instead of storing it in a second `useState` synchronized by an effect. If the current render's derived value depends on the *previous* render's props (e.g., resetting a selection when a list changes), prefer storing an `ID` and comparing it inline during render over an effect.
   - **Yes, and the goal is resetting all state when an identity-like prop changes** (e.g., `userId`, `itemId`) → this is the documented "Resetting state with an Effect" anti-pattern. Fix: pass the identity value as the `key` prop on the component (or a wrapped inner component) so React remounts and resets state automatically, instead of an effect that calls `setState(initialValue)`.
   - **No** → continue to question 3.
3. **Does the effect run logic that should only happen in response to a specific user action** (a click, a form submission) rather than in response to a state/prop change?
   - **Yes** → this is the documented "event-handler logic misplaced in an effect" anti-pattern (e.g., sending an analytics event or a mutation inside an effect that fires whenever some state changes, instead of inside the handler that caused the change). Fix: move the logic into the event handler. If the same logic must run regardless of which handler triggered the state change, that is one of the few legitimate reasons to keep it in an effect — but confirm that is actually true before accepting it as an exception.
   - **No** → continue to question 4.
4. **Does the effect exist solely to run initialization logic once on mount** (e.g., `loadDataFromLocalStorage()`, `checkAuthToken()`) at the top level of `App`?
   - **Yes** → flag it. In development with `StrictMode`, this class of effect runs twice, which is documented as an intentional signal that the logic was not designed to be resilient to remount. Fix depends on the actual intent: module-level code that should run once per page load (not per component mount) usually does not belong in an effect at all; if it must be effect-based and idempotency truly cannot be achieved, a `useRef` guard (`if (ranOnce.current) return; ranOnce.current = true;`) is the documented workaround, not a first choice.

Do not stop at the first "yes" in isolation without checking file:line evidence — cite the exact code that establishes the classification.

## Output contract

Every response must return:

1. **Scope** — the component(s), files, and hook call sites reviewed (file:line for each).
2. **Evidence level** — per finding: `repo evidence` (cited file:line), `documentation-based` (cites the specific react.dev page/section via Context7), or `inference` (a plausible but unconfirmed race/loop that needs live reproduction to prove).
3. **Ranked findings** — for each: anti-pattern category (one of the four decision-tree branches, or "missing cleanup guard", or "stale closure"), file:line, the concrete trigger sequence for any race/loop claim, and a fix sketch that matches the docs' recommended alternative (not a generic "add a check" — name the actual pattern: compute during render, reset via `key`, move to handler, add `ignore` flag/`AbortController`, add missing dependency, or `useEffectEvent` if the React version supports it).
4. **Verdict** — `approve`, `approve-with-notes`, or `block`. Block only for HIGH-severity findings (confirmed infinite loop, or a race condition on an authenticated write/mutation).
5. **Open questions** — anything requiring live reproduction (network timing, StrictMode remount behavior in the target environment) that static review cannot confirm; state this explicitly rather than asserting a bug "will occur" without evidence of the trigger sequence.

## Hard stops

- Do not recommend removing an effect that genuinely synchronizes with an external system. That is valid effect usage per docs, not an anti-pattern — removing it breaks the synchronization.
- Do not flag every `useEffect` call site as suspicious by default. Each finding must map to a specific decision-tree branch with file:line evidence.
- Do not claim a race condition "will occur" without describing the concrete input sequence (which two operations race, in what order, and what the observable symptom is).