fix(react-doctor): tighten state-and-effects rules against false positives#172
Merged
fix(react-doctor): tighten state-and-effects rules against false positives#172
Conversation
…tives Audit pass on `main` after the consolidation/AGENTS.md cleanups (#167, #169). Targets eight specific FP/FN patterns surfaced by re-reading the merged code end-to-end and checking each rule against its own intent docstring. High - H1 unify release detection between `effectHasCleanupRelease` and `cleanupReleasesSubscription`: both now share `isCleanupReturn` / `isReleaseLikeCall` / `containsReleaseLikeCall`. Previously `prefer-use-sync-external-store` and `prefer-use-effect-event` could disagree on whether a cleanup with a generic teardown verb (`cleanup()`, `dispose()`) counted as a release, producing inconsistent diagnostics. - H2 require function-shaped return for `isExternalSyncEffect`: any `return <expr>` previously qualified the effect as "external sync" and silently disabled chain detection, so `return null` / `return 42` / `return condition && cleanup` would mask real chains. We now only treat function-shaped returns (Arrow, FnExpr, Call, Identifier) as cleanup. - H3 directional version gating: `prefer-use-effect-event` is a "prefer-newer-api" rule and was firing on projects where the React major couldn't be detected. Gating now records whether each rule is "prefer-newer-api" (skip when version is unknown) or "deprecation-warning" (keep firing when version is unknown). Medium - M1 receiver-gate ambiguous direct callees: `track`, `logEvent`, `del` removed from `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES` — they were too generic as bare identifiers and produced FPs on user-defined helpers. Receiver-gated member calls (`analytics.track(...)`) still fire. - M2 extend `findSubHandlerForEnclosingFunction` to FunctionDeclaration and AssignmentExpression shapes — `function handler() {}` and `let h; h = (e) => {}` are now recognized alongside `const h = ...`. - M3 deep-walk for `noPropCallbackInEffect`: the rule previously only scanned top-level effect statements and missed the very common `if (didChange) onChange(state)` shape. Walk now descends through control-flow blocks but stops at function boundaries (so deferred sub-handlers like `setTimeout(() => onChange(state))` stay the domain of `prefer-use-effect-event`). - M4 `collectWrittenStateNamesInEffect` no longer counts setter calls inside nested functions for chain detection — deferred writes (`setTimeout(() => setX(...))`) are not synchronous chain triggers. Low - L1 `noMirrorPropEffect` now accepts multi-element deps as long as the mirrored prop's root is in the deps array. The prior "exactly one dep" requirement missed legitimate mirror shapes with an unrelated extra dep. - L2 (paired with H1/H2) `effectHasCleanupRelease` "return Identifier" now narrows to known bound release names so a stray non-function identifier return doesn't silently look like a release. - L3 extend `prop-stack-barrier.test.ts` to cover all four `createComponentPropStackTracker` consumers (`no-derived-useState`, `no-prop-callback-in-effect`, `no-mirror-prop-effect`, `prefer-use-effect-event`) so the empty-frame barrier semantic is regression-tested for every rule that depends on it. - L4 extract a sibling `createComponentBindingStackTracker` and migrate `noEffectEventInDeps` onto it — the third inlined push/pop scaffold was effectively a copy-paste of the prop-stack one specialised to binding sets. Tests - 634 passing (up from 619). New regressions cover each behavior change above plus FP guards (e.g. mirror shape with prop root NOT in deps must not fire, sub-handler reads must not fire `no-prop-callback-in-effect`). - `tests/run-oxlint.test.ts` now passes `reactMajorVersion: 19` for the basic / Next / TanStack Start fixtures so they exercise the prefer-newer-api rules under a known React major. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
4 tasks
aidenybai
added a commit
that referenced
this pull request
May 8, 2026
…() + cleanups H1 (regression introduced by #172) — `diagnose()` in `src/index.ts` forgot to forward `reactMajorVersion` to `runOxlint`. After the directional version-gating change in #172, that meant every "prefer-newer-api" rule (today: `prefer-use-effect-event`) was silently skipped for every programmatic API consumer, even on React 19+ projects. The CLI (`scan.ts`) was unaffected because it always passed the version explicitly. Fix is one line + one import — mirror what `scan.ts` already does. Added `tests/diagnose.test.ts` with a regression test that asserts `prefer-use-effect-event` fires on a React 19 fixture, plus a symmetric guard that it stays skipped when the React version can't be parsed (e.g. a github: range). H2 — updated the stale docstring on `runOxlint`'s `reactMajorVersion` field. The doc still claimed "`null` means unknown — leave those rules enabled" but after #172 the null branch is directional (deprecation-warning rules stay on, prefer-newer-api rules go off). M1 — `SUB_HANDLER_DIRECT_CALLEE_NAMES` was just an alias of `TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES`. Both names existed for narrative reasons in different files; knip flagged the duplicate export. Collapsed to the canonical name and updated the one consumer (`isCallExpressionWithSubHandlerCallee` in state-and-effects). L1 — moved `walkInsideStatementBlocks` from inline in `state-and-effects.ts` to `plugin/helpers.ts` next to its sibling `walkAst`. It already had four call sites and is the natural "synchronous-only" walker for any rule asking what runs inside an effect's own body — colocating with `walkAst` makes future rules discover it. L2 (audit finding) — proposed receiver-gating `post`/`put`/`patch` in `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES` INTENTIONALLY NOT TAKEN. The canonical "You Might Not Need an Effect" §6 example is `post(jsonToSubmit)` as a bare callee, so removing those names breaks textbook detection (3 existing tests / fixtures). Documented the trade-off in the constants.ts docstring. Validation - 642 tests passing (640 baseline + 2 new diagnose-API regressions) - typecheck / lint / format clean Co-authored-by: Cursor <cursoragent@cursor.com>
aidenybai
added a commit
that referenced
this pull request
May 8, 2026
…() + cleanups (#174) * fix(react-doctor): forward reactMajorVersion in programmatic diagnose() + cleanups H1 (regression introduced by #172) — `diagnose()` in `src/index.ts` forgot to forward `reactMajorVersion` to `runOxlint`. After the directional version-gating change in #172, that meant every "prefer-newer-api" rule (today: `prefer-use-effect-event`) was silently skipped for every programmatic API consumer, even on React 19+ projects. The CLI (`scan.ts`) was unaffected because it always passed the version explicitly. Fix is one line + one import — mirror what `scan.ts` already does. Added `tests/diagnose.test.ts` with a regression test that asserts `prefer-use-effect-event` fires on a React 19 fixture, plus a symmetric guard that it stays skipped when the React version can't be parsed (e.g. a github: range). H2 — updated the stale docstring on `runOxlint`'s `reactMajorVersion` field. The doc still claimed "`null` means unknown — leave those rules enabled" but after #172 the null branch is directional (deprecation-warning rules stay on, prefer-newer-api rules go off). M1 — `SUB_HANDLER_DIRECT_CALLEE_NAMES` was just an alias of `TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES`. Both names existed for narrative reasons in different files; knip flagged the duplicate export. Collapsed to the canonical name and updated the one consumer (`isCallExpressionWithSubHandlerCallee` in state-and-effects). L1 — moved `walkInsideStatementBlocks` from inline in `state-and-effects.ts` to `plugin/helpers.ts` next to its sibling `walkAst`. It already had four call sites and is the natural "synchronous-only" walker for any rule asking what runs inside an effect's own body — colocating with `walkAst` makes future rules discover it. L2 (audit finding) — proposed receiver-gating `post`/`put`/`patch` in `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES` INTENTIONALLY NOT TAKEN. The canonical "You Might Not Need an Effect" §6 example is `post(jsonToSubmit)` as a bare callee, so removing those names breaks textbook detection (3 existing tests / fixtures). Documented the trade-off in the constants.ts docstring. Validation - 642 tests passing (640 baseline + 2 new diagnose-API regressions) - typecheck / lint / format clean Co-authored-by: Cursor <cursoragent@cursor.com> * chore: format scripts/benchmark-scores.ts Pre-existing formatting issue introduced in 6afdc04 — the script was committed unformatted, breaking `pnpm format:check` on every PR branched off main since. Picked up by rebasing this PR. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit pass on
mainafter the consolidation/AGENTS.md cleanups (#167, #169). Targets eight specific FP/FN patterns surfaced by re-reading the merged code end-to-end and checking each rule against its own intent docstring. Plus two low-priority code-org cleanups (L3/L4) so all four prop-stack-tracker consumers are regression-tested for the empty-frame barrier and the binding-stack scaffold is extracted from a third inlined copy.High
effectHasCleanupReleaseandcleanupReleasesSubscription: both now shareisCleanupReturn/isReleaseLikeCall/containsReleaseLikeCall. Previouslyprefer-use-sync-external-storeandprefer-use-effect-eventcould disagree on whether a cleanup with a generic teardown verb (cleanup(),dispose()) counted as a release.isExternalSyncEffect: anyreturn <expr>previously qualified the effect as "external sync" and silently disabled chain detection, soreturn null/return 42would mask real chains. We now only treat function-shaped returns as cleanup.prefer-use-effect-eventis a "prefer-newer-api" rule and was firing on projects where the React major couldn't be detected. Gating now records each rule as either"prefer-newer-api"(skip when version is unknown) or"deprecation-warning"(keep firing when version is unknown).Medium
track,logEvent,delremoved fromEVENT_TRIGGERED_SIDE_EFFECT_CALLEES— they were too generic as bare identifiers. Receiver-gated member calls (analytics.track(...)) still fire.findSubHandlerForEnclosingFunctionto FunctionDeclaration and AssignmentExpression shapes —function handler() {}andlet h; h = (e) => {}are now recognized alongsideconst h = ....noPropCallbackInEffect: previously only top-level effect statements were scanned. The very commonif (didChange) onChange(state)shape was a silent FN. Walk now descends through control-flow blocks but stops at function boundaries.collectWrittenStateNamesInEffectno longer counts setter calls inside nested functions for chain detection — deferred writes (setTimeout(() => setX(...))) are not synchronous chain triggers.Low
noMirrorPropEffectnow accepts multi-element deps as long as the mirrored prop's root is in the deps array (was: exactly one dep).effectHasCleanupRelease"return Identifier" narrows to known bound release names so a stray non-function identifier return doesn't silently look like a release.prop-stack-barrier.test.tsto all fourcreateComponentPropStackTrackerconsumers (no-derived-useState,no-prop-callback-in-effect,no-mirror-prop-effect,prefer-use-effect-event) so the empty-frame barrier is regression-tested for every consumer.createComponentBindingStackTrackerand migratenoEffectEventInDepsonto it — the third inlined push/pop scaffold was effectively a copy-paste of the prop-stack one specialized to binding sets.Test plan
pnpm typecheck— cleanpnpm lint— 0 warnings, 0 errorspnpm format:check— cleanpnpm test— 634 passing (up from 619). New regressions cover each behavior change plus explicit FP guards:setTimeout(() => onChange(state))must not fireno-prop-callback-in-effect(M3)track(progress)user-defined helper must not fire (M1)analytics.track(progress)still fires (M1)useSyncExternalStorereimplementation with genericcleanup()is correctly detected (H1/L2)prefer-use-effect-eventdoes NOT fire whenreactMajorVersionis null (H3)Made with Cursor
Note
Medium Risk
Modifies multiple core lint-rule heuristics (effects/state analysis and version gating), which can change diagnostics and scoring behavior across projects. Changes are well-covered by new/expanded regression tests but still carry behavior-shift risk.
Overview
Improves several
react-doctorstate/effects rules to reduce false positives/negatives by refining AST traversal and gating logic.Adds directional React-version gating: version-gated rules now specify whether they should run when React version is unknown, so
prefer-use-effect-eventis suppressed onreactMajorVersion: nullwhile React 19 deprecation warnings still run.Tightens rule heuristics:
no-prop-callback-in-effectnow deep-walks control-flow blocks but stops at nested functions;no-effect-chainignores deferred setter writes and no longer treats non-functionreturnvalues as cleanup;no-mirror-prop-effectsupports multi-dep arrays when the mirrored prop is included; cleanup/release detection is unified via shared helpers soprefer-use-sync-external-storeandeffect-needs-cleanupagree (including generic teardown verbs).Extracts a reusable
createComponentBindingStackTracker, narrowsno-event-trigger-statedirect-callee allowlist to avoid generic-verb FPs, expands sub-handler binding detection forprefer-use-effect-event, and updates/extends regression tests (including setting testreactMajorVersiondefaults).Reviewed by Cursor Bugbot for commit b7cfd37. Bugbot is set up for automated code reviews on this repo. Configure here.