Skip to content

feat: add prefer-use-effect-event rule#162

Merged
aidenybai merged 3 commits intomainfrom
cursor/prefer-use-effect-event-7144
May 8, 2026
Merged

feat: add prefer-use-effect-event rule#162
aidenybai merged 3 commits intomainfrom
cursor/prefer-use-effect-event-7144

Conversation

@aidenybai
Copy link
Copy Markdown
Member

@aidenybai aidenybai commented May 7, 2026

New rule (severity: warn) inspired by Vercel Labs' advanced-use-latest and React's Separating Events from Effects.

What it catches

When a function-typed reactive value is read from an effect only inside a sub-handler, listing it in the dep array forces the whole effect to re-synchronize on every parent render. The fix is useEffectEvent:

function SearchInput({ onSearch }) {
  const [query, setQuery] = useState('');
  useEffect(() => {
    const id = setTimeout(() => onSearch(query), 300);  // ← sub-handler
    return () => clearTimeout(id);
  }, [query, onSearch]);                                 // 🔴 onSearch in deps
}
function SearchInput({ onSearch }) {
  const [query, setQuery] = useState('');
  const onSearchEvent = useEffectEvent(onSearch);
  useEffect(() => {
    const id = setTimeout(() => onSearchEvent(query), 300);
    return () => clearTimeout(id);
  }, [query]);                                           // ✅
}

Detector — four pre-conditions, all must hold

Chosen to keep real-world false positives near zero:

  1. useEffect with a dep array of ≥ 2 Identifier elements — single-dep effects don't meaningfully benefit from the migration.
  2. At least one dep is function-typed:
    • a destructured prop (uses the existing prop-stack scaffold), or
    • a local declared via const F = useCallback(...)
  3. Every read of F inside the effect body sits inside a sub-handler:
    • direct callee: setTimeout / setInterval / requestAnimationFrame / requestIdleCallback / queueMicrotask
    • member callee: subscribe / addEventListener / addListener / on / watch / listen / sub
    • the sub-handler can be passed inline (setTimeout(() => …)) or via a const-bound local (const handler = () => …; window.addEventListener('keydown', handler);)
  4. F is NEVER read at the effect's own top level — a synchronous call there is a true reactive read; useEffectEvent isn't appropriate.

React version gating

useEffectEvent is a React 19+ Hook. Recommending it on React 18 (or older) projects produces noisy diagnostics for an API the user doesn't have, so the rule is registered in the existing VERSION_GATED_RULE_IDS map under a new USE_EFFECT_EVENT_MIN_MAJOR = 19 constant.

Detected reactMajorVersion Behavior
19 (or higher) rule fires
18, 17 (or lower) rule is filtered out before oxlint runs
null (couldn't detect — workspace tag, missing dep, exotic spec) rule fires — same convention as every other version-gated rule, so we never silently swallow real findings

Tests — 12 regression cases

Detector behavior (8):

  • ✅ flags canonical setTimeout shape (Vercel advanced-use-latest)
  • ✅ flags addEventListener with const-bound handler + multi-deps
  • ✅ flags store.subscribe handler that calls a prop callback
  • ✅ flags useCallback-bound local invoked only from setInterval
  • ❌ does NOT flag callback read at the effect's top level (true reactive read)
  • ❌ does NOT flag when the dep is not function-typed (state, plain identifier)
  • ❌ does NOT flag a single-dep effect (no benefit)
  • ❌ does NOT flag addEventListener with the callback as the only dep

Version gating (4):

  • ✅ explicit reactMajorVersion: 19 → fires
  • ❌ explicit reactMajorVersion: 18 → does not fire
  • ❌ explicit reactMajorVersion: 17 → does not fire
  • reactMajorVersion: null (unknown) → fires

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

Coverage map for the useEffect track

After this PR, the comprehensive useEffect analyzer family across all open PRs covers:

Article / source Rule
§1 mirroring no-derived-state-effect, no-derived-useState, no-mirror-prop-effect (#157)
§2 expensive caching no-derived-state-effect memo branch (#153)
§3 / §4 reset/adjust no-derived-state-effect, no-set-state-in-render
§5 shared logic no-effect-event-handler widened (#153)
§6 trigger state no-event-trigger-state (#155)
§7 chains no-effect-chain (#156)
§9 / §10 parent notify no-prop-callback-in-effect
§11 external store prefer-use-sync-external-store (#154)
§12 fetching no-fetch-in-effect
Separating Events from Effects prefer-use-effect-event (this PR, React 19+)
Removing Effect Dependencies (mutable deps) no-mutable-in-deps (#157)
Removing Effect Dependencies (functional setState spread) rerender-functional-setstate extended (#157)
Removing Effect Dependencies (inline-fn deps) rerender-dependencies extended (#157)
Lifecycle of Reactive Effects (cleanup) effect-needs-cleanup (#157)

Checks

493/493 tests passing locally. Lint, typecheck, format clean. No changeset (per project preference).

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:22am

Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts Outdated
Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR's describe block to main's version + the
reactMajorVersion helper-arg modification. All 555 tests pass.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
@cursor cursor Bot force-pushed the cursor/prefer-use-effect-event-7144 branch from e02a323 to 6858ff4 Compare May 8, 2026 09:02
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts
@cursor cursor Bot force-pushed the cursor/prefer-use-effect-event-7144 branch from 6858ff4 to 8d89540 Compare May 8, 2026 09:16
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 8d89540. Configure here.

Comment thread packages/react-doctor/src/oxlint-config.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@aidenybai aidenybai merged commit 945138d into main May 8, 2026
4 of 5 checks passed
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