Skip to content

feat: add no-effect-chain rule#156

Merged
aidenybai merged 8 commits intomainfrom
cursor/no-effect-chain-7144
May 8, 2026
Merged

feat: add no-effect-chain rule#156
aidenybai merged 8 commits intomainfrom
cursor/no-effect-chain-7144

Conversation

@aidenybai
Copy link
Copy Markdown
Member

New rule (severity: warn) flagging the §7 anti-pattern from React's You Might Not Need an Effect guide.

What it catches

useEffect(() => { if (card?.gold) setGoldCount(c => c + 1); }, [card]);
useEffect(() => { if (goldCount > 3) setRound(r => r + 1); }, [goldCount]);
useEffect(() => { if (round > 5) setIsGameOver(true); }, [round]);

Each link adds an extra render to the tree below the component. The chain is also rigid — setting card to a value from the past re-fires every downstream effect. The fix is to compute as much as possible during render (const isGameOver = round > 5) and write all related state inside the event handler that originally fires the chain.

Detector

For every component body:

  1. Collect every top-level useEffect call and extract:
    • depNames: identifier names in the dep array
    • writtenStateNames: state names whose setter is called inside
    • isExternalSync: the effect returns a cleanup function or contains a recognized external-system call (subscribe, addEventListener, fetch, setInterval, new MutationObserver, etc.) or mutates a ref (ref.current = …)
  2. For every ordered pair (A, B) of distinct effects, draw an edge iff writes(A) ∩ deps(B) ≠ ∅ and neither A nor B is isExternalSync.
  3. Report on every reader effect that is the target of any edge, naming the chained state.

Complements existing no-cascading-set-state

no-cascading-set-state catches multi-setter calls inside one effect. no-effect-chain catches chains across effects. They detect orthogonal shapes and can fire independently.

Article's GOOD exception is honored

The article explicitly notes that a chain of effects is appropriate when each effect synchronizes with the network (e.g. cascading dropdowns where each fetches options for the next). Each fetch-bearing effect has isExternalSync = true and is exempt.

Tests — 5 regression cases

  • ✅ flags article §7 Game-style three-effect chain (≥2 hits, naming both goldCount and round as the chained values)
  • ❌ does NOT flag a single effect with multiple setters (covered by no-cascading-set-state)
  • ❌ does NOT flag the article's GOOD network-cascade ShippingForm (each effect calls fetch)
  • ❌ does NOT flag a chat-connection effect that shares deps with a state-reset effect (createConnection().connect() is external sync)
  • ❌ does NOT flag two effects whose written/read state sets are disjoint

Plus a smoke test in run-oxlint.test.ts against an EffectChainComponent fixture.

Constants added

  • EXTERNAL_SYNC_MEMBER_METHOD_NAMESsubscribe, addEventListener, connect, fetch, post, put, patch, delete, on, watch, listen, sub, addListener, disconnect, open, close
  • EXTERNAL_SYNC_DIRECT_CALLEE_NAMESfetch, ky, got, wretch, ofetch, setInterval, setTimeout, requestAnimationFrame, requestIdleCallback, queueMicrotask
  • EXTERNAL_SYNC_OBSERVER_CONSTRUCTORSIntersectionObserver, MutationObserver, ResizeObserver, PerformanceObserver

Checks

486/486 tests passing locally. Lint, typecheck, format clean. Changeset included (minor bump).

Open in Web Open in Cursor 

cursoragent and others added 2 commits May 7, 2026 11:25
New rule (severity: warn) flagging the §7 anti-pattern from React's
'You Might Not Need an Effect' guide:

  useEffect(() => { if (card.gold) setGoldCount(c => c + 1); }, [card]);
  useEffect(() => { if (goldCount > 3) setRound(r => r + 1); }, [goldCount]);
  useEffect(() => { if (round > 5) setIsGameOver(true); }, [round]);

Each link adds an extra render to the tree below the component.
The chain is also rigid — setting `card` to a value from the past
re-fires every downstream effect.

Detector (per component body):

  1. Collect every top-level useEffect call and, for each:
       - depNames: Identifier names in the dep array
       - writtenStateNames: state names whose setter is called in body
       - isExternalSync: body returns cleanup OR contains a recognized
         external-system call (subscribe / addEventListener / fetch /
         setInterval / new MutationObserver / etc.) OR mutates a ref
  2. For every ordered pair (A, B) of distinct effects:
       edge iff (writes(A) ∩ deps(B)) ≠ ∅
                AND ¬isExternalSync(A) AND ¬isExternalSync(B)
  3. Report on every reader effect B with the chained state name.

The article calls out one legitimate 'chain' — a multi-step network
cascade where each effect re-fetches based on the previous step's
result. Those effects all have isExternalSync=true (they contain
fetch), so the rule won't fire.

Complements the existing no-cascading-set-state rule (intra-effect
multi-setter detector) without overlapping.

Tests: 5 regression cases.
  flags:
    - article §7 Game-style three-effect chain
  does NOT flag:
    - single effect with multiple setters (covered by no-cascading-set-state)
    - article's GOOD network-cascade exception (each effect calls fetch)
    - chat-connection effect that shares deps with state-reset effect
      (createConnection().connect() / .disconnect() is external sync)
    - two effects whose written/read state sets are disjoint

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
react-doctor-website Ready Ready Preview, Comment May 8, 2026 9:01am

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Comment thread packages/react-doctor/src/plugin/constants.ts
…ternal sync

Bugbot found that EXTERNAL_SYNC_MEMBER_METHOD_NAMES included `post`,
`put`, `patch`, `delete`, `fetch` but omitted `get`. Read-only
network calls inside an effect (`axios.get(...)`, `ky.get(...)`,
`api.get(...)`) were therefore classified as internal-only sync,
producing false-positive chain warnings on the article's exact
'cascade of network fetches' exception.

Added `get`, `head`, and `options` to round out the standard
HTTP method set. Now an `axios.get`-bearing effect correctly has
`isExternalSync = true` and the rule won't flag it as a chain.

Regression test mirrors the article's ShippingForm example but with
`axios.get` instead of `fetch`, asserting zero hits.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Comment thread packages/react-doctor/src/plugin/constants.ts Outdated
… known client receivers

Bugbot caught: round 1 added `get` to EXTERNAL_SYNC_MEMBER_METHOD_NAMES
to support `axios.get` cascades, but `get` is also a universal
data-structure method name (`Map.get`, `URLSearchParams.get`,
`FormData.get`, `Headers.get`, `WeakMap.get`). Effects whose only
'external' call was `params.get('id')` were misclassified as external
sync, causing false-NEGATIVE chain detection.

Two new constants split the ambiguity:

  EXTERNAL_SYNC_MEMBER_METHOD_NAMES — unambiguous methods (subscribe,
    addEventListener, post, put, patch, delete, fetch, …). Receiver
    is irrelevant; the method name alone marks the call as external.

  EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES — the HTTP-vs-data-structure
    overlapping verbs (get, head, options). Only count as external
    when paired with a recognized HTTP-client receiver name.

  EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS — axios, ky, got, wretch,
    ofetch, api, client, http, request, fetcher.

Regression test asserts that a chain where one effect calls
`params.get('theme')` (URLSearchParams) still produces the chain
warning, verifying that data-structure-shape `.get` no longer
exempts the effect.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Comment thread packages/react-doctor/src/plugin/constants.ts Outdated
#156 round 3)

Bugbot caught: round-2 left `delete` in EXTERNAL_SYNC_MEMBER_METHOD_NAMES
(unambiguous external-sync) while moving `get`, `head`, `options`
to the ambiguous set. But `delete` has the same problem — it's the
name of standard JS APIs on Map, Set, URLSearchParams, Headers,
FormData, and WeakMap. An effect whose only 'external' call was
`set.delete(item)` got falsely classified as external sync,
silently exempting it from chain detection.

Move `delete` to EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES so it's
only counted as external when paired with a recognized HTTP-client
receiver (axios, ky, …). `set.delete()`, `map.delete()`, etc. now
correctly stay classified as internal-only.

Regression test: a chain where one effect calls `next.delete(...)`
on a Set still produces the chain warning.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 161946b. Configure here.

Comment thread packages/react-doctor/tests/regressions/state-rules.test.ts
…cise isExternalSync (Bugbot #156 round 4)

Bugbot caught: the cascade / ShippingForm / chat tests previously
had no actual write→dep state overlap between their effects, so
they passed regardless of whether `isExternalSync` worked correctly.
Reverting the isExternalSync exemption would NOT have caused those
tests to fail — they were vacuously green.

All three rewritten so the second effect depends on a state that
the first effect writes:

  ShippingForm: writes(A) = {cities}, deps(B) = [cities]
  axios cascade: writes(A) = {cities}, deps(B) = [cities]
  chat: writes(A) = {status}, deps(B) = [status]

In every case both effects are external-sync (fetch / axios.get /
createConnection().connect() / addEventListener), so the chain
detector exempts them. If the exemption breaks, all three tests
will fail — the chain detection itself fires correctly per the
existing intra-effect tests.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@aidenybai aidenybai force-pushed the cursor/no-effect-chain-7144 branch from d9496ef to e5ecbb5 Compare May 8, 2026 09:01
@aidenybai aidenybai merged commit 4b92f50 into main May 8, 2026
5 checks passed
cursor Bot pushed a commit that referenced this pull request May 8, 2026
Rebased onto main after #154 / #155 / #156 merged. Clean reapply via
patch — constants, oxlint-config, run-oxlint, fixtures, tests, and
state-and-effects all applied without conflicts. Re-added
USE_EFFECT_EVENT_MIN_MAJOR which got dropped in an earlier merge.
All 555 tests pass.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…eps, no-mirror-prop-effect, effect-needs-cleanup)

Rebased onto main after #154 / #155 / #156 merged. Clean reapply via
patch — all 9 files applied without conflicts. All 600 tests pass.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
aidenybai added a commit that referenced this pull request May 8, 2026
…eps, no-mirror-prop-effect, effect-needs-cleanup) (#157)

Rebased onto main after #154 / #155 / #156 merged. Clean reapply via
patch — all 9 files applied without conflicts. All 600 tests pass.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants