Skip to content

feat: comprehensive useEffect analyzer expansion (3 rules + 2 extensions)#157

Merged
aidenybai merged 1 commit intomainfrom
cursor/comprehensive-useeffect-analyzer-7144
May 8, 2026
Merged

feat: comprehensive useEffect analyzer expansion (3 rules + 2 extensions)#157
aidenybai merged 1 commit intomainfrom
cursor/comprehensive-useeffect-analyzer-7144

Conversation

@aidenybai
Copy link
Copy Markdown
Member

Three new rules and two detector extensions targeted at the gaps between the existing rule set and React's full effect-related guidance. Builds on the smaller follow-up PRs already in flight (#153 detector widening, #154 prefer-use-sync-external-store, #155 no-event-trigger-state, #156 no-effect-chain) without overlapping them.

New rules

no-mirror-prop-effect (warn)

Flags the canonical "mirror a prop into local state via a useEffect" anti-pattern from §1 of You Might Not Need an Effect:

function Form({ value }) {
  const [draft, setDraft] = useState(value);              // mirror
  useEffect(() => { setDraft(value); }, [value]);         // sync
}

Both no-derived-state-effect and no-derived-useState already nudge at parts of this; this rule produces a single, more actionable diagnostic that names the prop and recommends deleting both. Detector reuses the prop-stack scaffolding from no-prop-callback-in-effect / no-derived-useState. Recognizes both useState(prop) and useState(prop.x) MemberExpression variants. Structural equality between the useState initializer and the setX(...) argument is required, so setDraft(value.toUpperCase()) (a transformation, not a mirror) is correctly not flagged.

no-mutable-in-deps (error)

Flags mutable values in a hook's deps array — they don't trigger re-runs because mutations happen outside React's data flow. From Lifecycle of Reactive Effects §"Can global or mutable values be dependencies?":

Mutable values aren't reactive. Even if you specified it in the dependencies, React wouldn't know to re-synchronize the Effect.

Two shapes:

  • MemberExpression rooted in a known mutable global — location.pathname, window.innerWidth, document.title, navigator.onLine, history.state, screen.width, performance.now
  • MemberExpression <x>.current where <x> is a useRef binding declared in the same component

Bare ref Identifiers (containerRef) and regular state.field reads (state IS reactive) are intentionally not flagged.

effect-needs-cleanup (error)

Flags useEffect bodies that subscribe or schedule but never return a cleanup. Without it the registration leaks on every effect re-run and on unmount; StrictMode in development surfaces this as "you forgot to clean up an effect."

Recognizes three subscribe-shaped families and three cleanup forms:

Subscribe family Cleanup family
addEventListener / subscribe / addListener / on / watch / listen / sub removeEventListener / unsubscribe / removeListener / off / unwatch / unlisten / unsub
setInterval / setTimeout clearInterval / clearTimeout
any of the above bare return unsubscribe (Identifier — caller bound it)
any of the above return () => unsubscribe() / cleanup() / dispose() / destroy() / teardown()

Detector extensions (no new rule IDs)

rerender-dependencies — flag inline function literals

The rule already flagged ObjectExpression and ArrayExpression in deps; an inline ArrowFunctionExpression / FunctionExpression has the same identity-instability problem. Closes a coverage gap from Removing Effect Dependencies §"Does some reactive value change unintentionally?".

rerender-functional-setstate — flag array/object spread

The rule already flagged arithmetic and ++ / --. The array-spread / object-spread shape is the most common stale-closure trap inside subscription handlers and is exactly what Removing Effect Dependencies §"Are you reading some state to calculate the next state?" addresses:

setMessages([...messages, receivedMessage]);  // 🔴 now flagged
setProfile({ ...profile, name: input });      // 🔴 now flagged

The recommended fix in both cases is the functional-updater shape (prev => [...prev, item]) which removes the need for the state in the deps array.

Tests

23 new regression cases across the five additions, covering both positive triggers and article-cited GOOD shapes that must NOT be flagged.

Rule / extension Flag cases No-flag cases
no-mirror-prop-effect 2 (Identifier prop, prop.x member) 3 (uncontrolled-with-key, derive-not-mirror, transform-not-mirror)
no-mutable-in-deps 3 (location.pathname, containerRef.current, window.innerWidth) 2 (bare ref Identifier, state.field MemberExpression)
effect-needs-cleanup 3 (addEventListener, setInterval, store.subscribe) 3 (bare-return, removeEventListener, clearInterval)
rerender-functional-setstate spread 2 (array spread, object spread) 2 (functional updater, unrelated spread source)
rerender-dependencies inline-fn 2 (arrow in useEffect, fn-expr in useCallback) 1 (stable Identifier deps)

Plus 5 fixture-level smoke tests in run-oxlint.test.ts.

Full suite: 506/506 passing locally. Lint, typecheck, format clean.

Coverage map after this PR

The full set of "You Might Not Need an Effect" sections now has detector coverage:

§ Anti-pattern Rule(s)
§1 Updating state based on props/state no-derived-state-effect, no-derived-useState, no-mirror-prop-effect (this PR)
§2 Caching expensive calculations no-derived-state-effect (memo branch in #153)
§3 Resetting all state on prop change no-derived-state-effect (reset branch)
§4 Adjusting some state on prop change no-set-state-in-render whitelist + no-derived-state-effect
§5 Sharing logic between event handlers no-effect-event-handler (widened in #153)
§6 Sending a POST request no-event-trigger-state (#155)
§7 Chains of computations no-effect-chain (#156)
§8 Initializing the application intentionally skipped (FP risk)
§9 / §10 Notifying parent / passing data to parent no-prop-callback-in-effect
§11 Subscribing to an external store prefer-use-sync-external-store (#154)
§12 Fetching data no-fetch-in-effect, query-no-query-in-effect, tanstack-start-no-useeffect-fetch

Plus reactivity-hygiene rules from Removing Effect Dependencies and Lifecycle of Reactive Effects: rerender-dependencies (objects, arrays, functions), no-mutable-in-deps, effect-needs-cleanup, rerender-functional-setstate (spread cases), no-effect-event-in-deps, advanced-event-handler-refs.

Open in Web Open in Cursor 

@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:17am

Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts Outdated
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts Outdated
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts Outdated
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…und 2)

Two interface fields were assigned at construction but never read by
any consumer:

  SubscribeLikeUsage.callExpression — assigned in
    findSubscribeLikeUsages but effectNeedsCleanup only reads .kind
    and .resourceName

  MirrorBinding.useStateDeclarator — assigned in noMirrorPropEffect
    but no downstream code reads it (the validator reports on the
    matching effectCall, not the useState declarator)

Both removed. No functional change.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…unsubscribe-name regex (Bugbot #157 round 3)

Two related fixes for false positives:

  1. `findSubscribeLikeUsages` walked the entire callback AST,
     including any returned cleanup function. A `setTimeout` inside
     `return () => { ... }` got counted as a new registration and
     then flagged for missing cleanup. The walker now identifies
     the cleanup ReturnStatement's argument up-front and prunes it
     from the traversal via walkAst's prune-on-false return.

  2. The Identifier-callee cleanup regex only matched the long-form
     names (`unsubscribe` / `cleanup` / `dispose` / `destroy` /
     `teardown`). The short-form `unsub` and the method-shape
     names already in UNSUBSCRIBE_LIKE_METHOD_NAMES (`off`,
     `unwatch`, `unlisten`, `removeListener`,
     `removeEventListener`) were missing, so:

       const unsub = store.subscribe(handler);
       return () => unsub();

     produced a false positive. Extended the regex to cover all the
     names the rule already documents.

Two regression tests cover both fixes.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts Outdated
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…ile scope (Bugbot #157 round 4)

The helper was inlined three times with identical bodies in
`noDerivedUseState`, `noPropCallbackInEffect`, and the newly added
`noMirrorPropEffect`. Bugbot pointed out the divergence risk (low
severity but a clear maintenance smell). Hoist to a single
file-scope const at the top of the module; the three rules now
share the implementation.

No behavior change. 511/511 tests still pass.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
@cursor cursor Bot force-pushed the cursor/comprehensive-useeffect-analyzer-7144 branch from 59353ea to 9325915 Compare May 8, 2026 08:56
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts
…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>
@cursor cursor Bot force-pushed the cursor/comprehensive-useeffect-analyzer-7144 branch from dd58d86 to 0bd6a16 Compare May 8, 2026 09:17
@aidenybai aidenybai merged commit 0be99ad into main May 8, 2026
6 checks passed
cursor Bot pushed a commit that referenced this pull request May 8, 2026
Rebased onto main after #157 merged. Clean reapply via patch — all 615 tests pass.

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 0bd6a16. Configure here.

while (cursor?.type === "MemberExpression") cursor = cursor.object;
if (cursor?.type === "Identifier" && MUTABLE_GLOBAL_ROOTS.has(cursor.name)) {
return { kind: "global", rootName: cursor.name };
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mutable-global check ignores local variable shadowing

High Severity

findMutableDepIssue flags any MemberExpression whose root identifier name is in MUTABLE_GLOBAL_ROOTS (e.g. location, history, navigator), but never checks whether that identifier is actually the browser global or a locally-scoped variable. React Router's useLocation() canonically binds to const location = useLocation(), and [location.pathname] in a deps array is a perfectly valid reactive dependency. The rule would report it at error severity as a "mutable global," producing a false positive in virtually every React Router codebase. The ref.current path already does scope analysis via collectUseRefBindingNames; the global path needs analogous shadowing detection.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0bd6a16. Configure here.

aidenybai added a commit that referenced this pull request May 8, 2026
…167)

Cleanup pass over the recently-merged state-and-effects rules from
#153#157, #162 surfacing six audit findings.

1. **De-duplicate constant sets.** Four overlapping allowlists
   (`SUBSCRIPTION_METHOD_NAMES` ⊂ `EXTERNAL_SYNC_MEMBER_METHOD_NAMES`,
   `FETCH_MEMBER_OBJECTS` ⊂ `EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS`,
   `FETCH_CALLEE_NAMES` ⊂ `EXTERNAL_SYNC_DIRECT_CALLEE_NAMES` &
   `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES`, and the timer/scheduler
   list shared between `prefer-use-effect-event` and
   `no-effect-chain`) now spread from a single base set so adding a
   new HTTP client / subscribe verb in one place propagates
   everywhere.

2. **Unify `getRootIdentifierName`.** `server.ts` had a private copy
   that walked through CallExpression chains; the shared helper in
   `helpers.ts` did not. Added an opt-in `followCallChains` option
   and deleted the duplicate.

3. **Remove dead branch in `isExternalSyncEffect`.** A redundant
   `else if (effectCallback.body?.type !== "BlockStatement")` whose
   body was a comment-only no-op.

4. **Migrate `noDerivedUseState` to the shared root walker.** Drops
   the inline MemberExpression chain walk and uses
   `getRootIdentifierName` for consistency.

5. **Drop `noEffectEventHandler`'s deference to `noEventTriggerState`.**
   The deference predicate only checked two of the five preconditions
   `noEventTriggerState` actually requires (single dep,
   handler-only writes, not render-reachable were not verified),
   so when those failed the narrower rule didn't fire AND this rule
   deferred — a silent drop. Both rules now fire independently; the
   two messages frame the same code differently and a duplicate
   diagnostic is strictly better than a missing one. Also removes
   ~60 lines of useState-name-tracking machinery that only existed
   to support the deference. Existing test was updated and a new
   regression pinning the double-warn was added.

6. **Receiver-gate `push` / `replace`.** Both names live in
   `EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS` but also denote
   `Array.prototype.push` and `String.prototype.replace`. Moved
   them to a separate `EVENT_TRIGGERED_NAVIGATION_METHOD_NAMES`
   set that's only counted when the receiver root is in
   `NAVIGATION_RECEIVER_NAMES` (`router`, `navigation`, `navigator`,
   `history`, `location`). Two regressions added — `arr.push(x)`
   does not fire, `router.push(path)` still does.

All checks green: 617/617 tests, typecheck, lint, format.

Co-authored-by: Cursor <cursoragent@cursor.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