Skip to content

feat(react-doctor): add no-event-trigger-state rule#155

Merged
aidenybai merged 8 commits intomainfrom
cursor/no-event-trigger-state-7144
May 8, 2026
Merged

feat(react-doctor): add no-event-trigger-state rule#155
aidenybai merged 8 commits intomainfrom
cursor/no-event-trigger-state-7144

Conversation

@aidenybai
Copy link
Copy Markdown
Member

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

What it catches

const [jsonToSubmit, setJsonToSubmit] = useState(null);
useEffect(() => {
  if (jsonToSubmit !== null) {
    post("/api/register", jsonToSubmit);
  }
}, [jsonToSubmit]);

function handleSubmit(event) {
  event.preventDefault();
  setJsonToSubmit({ firstName, lastName });
}

The state variable exists only to schedule an effect to run on click. The fix is to call post(...) directly inside handleSubmit and delete the state.

Detector — four pre-conditions, all must hold

Chosen to keep real-world false positives near zero:

  1. useEffect with a single-identifier dep array, where the dep is a useState binding declared in this component.
  2. effect body is exactly one IfStatement guarding on that state with one of:
    • bare truthy if (X)
    • !== null / !== undefined / != null
    • === Literal
    • X.length (truthy)
    • !X (negated)
  3. the if's consequent contains a CallExpression whose callee is in a small allowlist:
    • direct names: fetch, post, put, patch, del, ky, got, wretch, ofetch, navigate, navigateTo, showNotification, toast, alert, confirm, track, logEvent, logVisit, captureEvent
    • member-call methods: post, put, patch, delete, push, replace, navigate, capture, track, logEvent — covers axios.post, router.push, analytics.track, posthog.capture, etc.
  4. every setX(...) call site in the component is inside a JSX on* handler (or a function bound to one) — i.e. the trigger is set only by user interactions.

(4) is the strongest signal that the state exists only to schedule the effect, and is what distinguishes this rule from §5 (handled by the existing no-effect-event-handler).

What it does NOT flag

The article's legitimate analytics-on-mount effect:

useEffect(() => post("/analytics/event", { eventName: "visit_form" }), []);

is not flagged — empty deps, no trigger state, runs because the form was displayed.

State that's also written by other reactive logic (another effect, top-of-render adjustment) isn't flagged either — that's a different pattern.

Tests — 7 regression cases

  • ✅ flags canonical post-trigger shape
  • ✅ flags axios.post member-call shape
  • ✅ flags bare-truthy guard with navigate(...)
  • ❌ does NOT flag analytics-on-mount with empty deps
  • ❌ does NOT flag when state is also written outside handlers
  • ❌ does NOT flag when consequent has no recognized side-effect (compute(seed))
  • ❌ does NOT flag when the dep is a prop (no useState binding)

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

Reuse

Reuses existing collectHandlerBindingNames and isInsideEventHandler helpers already in the file (used by rerender-defer-reads-hook).

Checks

488/488 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:18
New rule (severity: warn) flagging the §6 anti-pattern from React's
'You Might Not Need an Effect' guide:

  const [jsonToSubmit, setJsonToSubmit] = useState(null);
  useEffect(() => {
    if (jsonToSubmit !== null) {
      post('/api/register', jsonToSubmit);
    }
  }, [jsonToSubmit]);

  function handleSubmit(event) {
    event.preventDefault();
    setJsonToSubmit({ firstName, lastName });
  }

The state variable exists only to schedule an effect to run on click.
The fix is to call `post('/api/register', { firstName, lastName })`
directly inside handleSubmit and delete the state — and that's exactly
what the rule's diagnostic recommends.

Detector pre-conditions (all four must hold) — chosen to keep
real-world false positives near zero:

  (1) useEffect with a single-identifier dep array, where the dep is
      a useState binding declared in this component
  (2) effect body is exactly one IfStatement guarding on that state
      with one of: bare truthy, !== null/undefined, === Literal,
      .length, or !X
  (3) IfStatement.consequent contains a CallExpression whose callee
      is in EVENT_TRIGGERED_SIDE_EFFECT_CALLEES (fetch, post, navigate,
      showNotification, alert, track, ...) OR a MemberExpression
      whose property is in EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS
      (`axios.post`, `router.push`, `analytics.track`, etc.)
  (4) every setStateX call site in the component is inside a JSX
      `on*` handler (or a function bound to one) — i.e. the trigger
      is set only by user interactions

(4) is the strongest signal that the state exists *only* to schedule
the effect, and is what distinguishes this rule from §5 (handled by
the existing no-effect-event-handler).

Reuses existing helpers `collectHandlerBindingNames` /
`isInsideEventHandler` from the same file.

Tests: 7 regression cases.
  flags:
    - canonical post-trigger shape
    - axios.post member-call shape
    - bare truthy guard with navigate(...)
  does NOT flag:
    - article's GOOD analytics-on-mount example (empty deps, no trigger)
    - state also written outside handlers (mixed reactive logic)
    - guard with a non-side-effect callee (compute(seed))
    - guard on a prop (no useState binding present)

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 8:48am

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts
…d in render

Bugbot found that the rule's four pre-conditions don't include a
render-reachability guard. State that's BOTH a controlled-input
value AND a trigger for the effect (e.g. `query` driving both
`<input value={query}>` and `useEffect(() => track('search', query),
[query])`) satisfied all four conditions and produced a false
positive — the message told the user to 'delete the state', which
would break the input.

Added the render-reachability check used by rerender-state-only-in-
handlers: collect return expressions → expand transitive
dependencies → check if the trigger state is render-reachable. If
yes, skip — it's dual-purpose, not a pure trigger.

Regression test mirrors the Bugbot example.

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
…— round 2 Bugbot follow-ups

Two further issues Bugbot caught after round 1:

  1. `getTriggerGuardRootName` returned the first Identifier on
     either side of a BinaryExpression. Since `undefined` is parsed
     as Identifier (not Literal like `null`), the reversed
     ordering `if (undefined !== pendingPayload)` returned
     `"undefined"` and silently dropped the violation. Now skip
     a curated SENTINEL_IDENTIFIER_NAMES set
     (undefined / NaN / null) so the actual state Identifier
     wins regardless of operand order.

  2. The bare-truthy guard `if (destination) navigate(destination)`
     produced TWO warnings on the same line: one from
     `no-effect-event-handler` (its original §5 detector) and one
     from the new `no-event-trigger-state`. The two messages overlap.
     Solution: `no-effect-event-handler` now defers to
     `no-event-trigger-state` when the dep is a useState value
     (which is what `no-event-trigger-state` was built to handle).
     For the canonical §5 case (prop-driven trigger like
     `if (isOpen) document.body...`), `no-effect-event-handler`
     still fires.

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
…rigger-state would actually fire (Bugbot #155 round 3)

Round 2 made no-effect-event-handler defer whenever the trigger
guard's dep was a useState value. But no-event-trigger-state has
narrower preconditions — single dep, handler-only writes, and
specifically a callee in EVENT_TRIGGERED_SIDE_EFFECT_CALLEES (or
EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS for member calls). For a
state-typed trigger guard whose consequent calls a custom function
NOT in those allowlists (`if (trigger) customAction()`), the
round-2 deference silently dropped the warning — neither rule fired.

Tighten the deference: only skip when BOTH predicates hold —
state-typed dep AND the consequent contains a recognized event-
triggered side-effect callee. Otherwise no-effect-event-handler
keeps owning the diagnostic, preserving the previous behavior for
custom-callee shapes.

Pulled the side-effect-callee detection into a reusable
`consequentCallsTriggeredSideEffect` helper that mirrors what
no-event-trigger-state's `findTriggeredSideEffectCalleeName`
already does. The two rules now stay aligned via the same shared
constants (EVENT_TRIGGERED_SIDE_EFFECT_CALLEES,
EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS).

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 b778bd8. Configure here.

Comment thread packages/react-doctor/src/plugin/rules/state-and-effects.ts Outdated
…o findTriggeredSideEffectCalleeName (Bugbot #155 round 4)

Round 3 introduced `consequentCallsTriggeredSideEffect` for the
deference logic, which duplicated the AST walk + constant lookups
from `findTriggeredSideEffectCalleeName` declared later in the
file. Bugbot pointed out that the boolean function is exactly
`findTriggeredSideEffectCalleeName(node) !== null`.

Drop the duplicate. Reuse the existing helper directly. Forward
reference works because the call site sits inside a closure
(noEffectEventHandler.create()'s returned visitor) — at the time
that visitor runs, all module-level functions are initialized.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@aidenybai aidenybai changed the title feat: add no-event-trigger-state rule feat(react-doctor): add no-event-trigger-state rule May 8, 2026
@aidenybai aidenybai merged commit 2240b1f into main May 8, 2026
6 checks passed
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…ndler defer

Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR #155's describe block to main's version. The
noEffectEventHandler rule now combines main's widened test predicate
(uses getRootIdentifierName so MemberExpression guards like
`if (product.isInCart)` match) with PR #155's deference logic
(skip when noEventTriggerState would fire — state-typed dep +
recognized side-effect callee). All 552 tests pass; lint, typecheck,
format clean.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
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