fix(react-doctor): tighten state-and-effects rules against false positives (#172)

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>

Author: aidenybai

packages/react-doctor/src/oxlint-config.ts

@@ -381,24 +381,50 @@ export const ALL_REACT_DOCTOR_RULE_KEYS: ReadonlySet<string> = new Set([
 
 // HACK: single source of truth for which rules are gated behind the
 // project's detected React major. Adding a new version-gated rule means
-// touching just this map. `null` reactMajorVersion (couldn't detect)
-// keeps every rule enabled so we never silently swallow real findings.
-const VERSION_GATED_RULE_IDS: ReadonlyMap<string, number> = new Map([
-  ["react-doctor/no-react19-deprecated-apis", REACT_19_DEPRECATION_MIN_MAJOR],
-  ["react-doctor/no-default-props", REACT_19_DEPRECATION_MIN_MAJOR],
-  ["react-doctor/no-react-dom-deprecated-apis", REACT_DOM_LEGACY_API_MIN_MAJOR],
-  ["react-doctor/prefer-use-effect-event", USE_EFFECT_EVENT_MIN_MAJOR],
+// touching just this map.
+//
+// `mode` controls behavior when version detection FAILS (null):
+//   - "prefer-newer-api": the rule recommends an API that ONLY exists at
+//     or above `minMajor` (e.g. `useEffectEvent`). Suggesting it on a
+//     project where we can't prove the API exists is noise — fail closed.
+//   - "deprecation-warning": the rule flags patterns that BREAK at or
+//     above `minMajor` (e.g. `defaultProps` removal in React 19). Useful
+//     even on projects we can't version-detect, because the user may be
+//     mid-migration. Fail open.
+type VersionGateMode = "prefer-newer-api" | "deprecation-warning";
+interface VersionGate {
+  minMajor: number;
+  mode: VersionGateMode;
+}
+const VERSION_GATED_RULE_IDS: ReadonlyMap<string, VersionGate> = new Map([
+  [
+    "react-doctor/no-react19-deprecated-apis",
+    { minMajor: REACT_19_DEPRECATION_MIN_MAJOR, mode: "deprecation-warning" },
+  ],
+  [
+    "react-doctor/no-default-props",
+    { minMajor: REACT_19_DEPRECATION_MIN_MAJOR, mode: "deprecation-warning" },
+  ],
+  [
+    "react-doctor/no-react-dom-deprecated-apis",
+    { minMajor: REACT_DOM_LEGACY_API_MIN_MAJOR, mode: "deprecation-warning" },
+  ],
+  [
+    "react-doctor/prefer-use-effect-event",
+    { minMajor: USE_EFFECT_EVENT_MIN_MAJOR, mode: "prefer-newer-api" },
+  ],
 ]);
 
 const filterRulesByReactMajor = (
   rules: Record<string, RuleSeverity>,
   reactMajorVersion: number | null,
 ): Record<string, RuleSeverity> => {
-  if (reactMajorVersion === null) return rules;
   return Object.fromEntries(
     Object.entries(rules).filter(([ruleKey]) => {
-      const minMajor = VERSION_GATED_RULE_IDS.get(ruleKey);
-      return minMajor === undefined || reactMajorVersion >= minMajor;
+      const gate = VERSION_GATED_RULE_IDS.get(ruleKey);
+      if (gate === undefined) return true;
+      if (reactMajorVersion === null) return gate.mode === "deprecation-warning";
+      return reactMajorVersion >= gate.minMajor;
     }),
   );
 };

packages/react-doctor/src/plugin/constants.ts

@@ -568,13 +568,19 @@ export const EXTERNAL_SYNC_OBSERVER_CONSTRUCTORS = new Set([
 // almost always denotes a fire-and-forget user-action effect."
 // Layered on top of `FETCH_CALLEE_NAMES` so adding a new HTTP client
 // shorthand in one place propagates to every detector that recognizes it.
+//
+// HACK: ambiguous generic verbs (`track`, `logEvent`, `del`) used to
+// live here too. They produced FPs on user-defined helpers
+// (`track(progress)`, `del(item)`) that have nothing to do with
+// network/analytics side effects. Detection still works via the
+// receiver-bound member-call shape (`analytics.track(...)`,
+// `api.del(...)`) in `EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS`.
 export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([
   ...FETCH_CALLEE_NAMES,
   // Network shorthand verbs (article uses `post`)
   "post",
   "put",
   "patch",
-  "del",
   // Navigation
   "navigate",
   "navigateTo",
@@ -584,8 +590,6 @@ export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([
   "alert",
   "confirm",
   // Analytics
-  "track",
-  "logEvent",
   "logVisit",
   "captureEvent",
 ]);

packages/react-doctor/src/plugin/helpers.ts

@@ -19,6 +19,17 @@ interface ComponentPropStackTracker {
   visitors: RuleVisitors;
 }
 
+interface ComponentBindingStackTrackerCallbacks {
+  onVariableDeclarator?: (node: EsTreeNode) => void;
+}
+
+interface ComponentBindingStackTracker {
+  isInsideComponent: () => boolean;
+  isBoundName: (name: string) => boolean;
+  addBindingToCurrentFrame: (name: string) => void;
+  visitors: RuleVisitors;
+}
+
 // HACK: AST is acyclic except for `parent` back-references, which we skip.
 // Visitors may return `false` to prune the subtree below `node` (e.g. to
 // stop walking into nested functions when collecting `await` expressions
@@ -476,3 +487,63 @@ export const createComponentPropStackTracker = (
 
   return { isPropName, getCurrentPropNames, visitors };
 };
+
+// HACK: sibling of `createComponentPropStackTracker` for rules that need
+// to track *binding* sets per component scope rather than the destructured
+// prop set — e.g. `no-effect-event-in-deps` accumulates the names of
+// `useEffectEvent` declarators while inside a component and then queries
+// "is this dep-array identifier one of our useEffectEvent bindings?".
+//
+// Three rules previously reimplemented this push/pop bookkeeping inline.
+// They now share the same scaffold; the per-rule predicate (e.g. "is the
+// initializer a `useEffectEvent(...)` call?") lives in the
+// `onVariableDeclarator` callback.
+//
+// The barrier semantic is intentionally simpler than the prop-stack
+// tracker: the rule (e.g. `no-effect-event-in-deps`) only mutates the
+// top frame for VariableDeclarators directly inside a component, and
+// the stack only grows on FunctionDeclaration / VariableDeclarator
+// component entries, so a closed-over name from an outer component
+// can't leak in via a nested helper.
+export const createComponentBindingStackTracker = (
+  callbacks?: ComponentBindingStackTrackerCallbacks,
+): ComponentBindingStackTracker => {
+  const componentBindingStack: Array<Set<string>> = [];
+
+  const isInsideComponent = (): boolean => componentBindingStack.length > 0;
+
+  const isBoundName = (name: string): boolean => {
+    for (let frameIndex = componentBindingStack.length - 1; frameIndex >= 0; frameIndex--) {
+      if (componentBindingStack[frameIndex].has(name)) return true;
+    }
+    return false;
+  };
+
+  const addBindingToCurrentFrame = (name: string): void => {
+    if (componentBindingStack.length === 0) return;
+    componentBindingStack[componentBindingStack.length - 1].add(name);
+  };
+
+  const visitors: RuleVisitors = {
+    FunctionDeclaration(node: EsTreeNode) {
+      if (!node.id?.name || !isUppercaseName(node.id.name)) return;
+      componentBindingStack.push(new Set());
+    },
+    "FunctionDeclaration:exit"(node: EsTreeNode) {
+      if (!node.id?.name || !isUppercaseName(node.id.name)) return;
+      componentBindingStack.pop();
+    },
+    VariableDeclarator(node: EsTreeNode) {
+      if (isComponentAssignment(node)) {
+        componentBindingStack.push(new Set());
+        return;
+      }
+      callbacks?.onVariableDeclarator?.(node);
+    },
+    "VariableDeclarator:exit"(node: EsTreeNode) {
+      if (isComponentAssignment(node)) componentBindingStack.pop();
+    },
+  };
+
+  return { isInsideComponent, isBoundName, addBindingToCurrentFrame, visitors };
+};