refactor(react-doctor): consolidate state-and-effects rule plumbing

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

@@ -390,18 +390,28 @@ export const MUTATING_ROUTE_SEGMENTS = new Set([
 export const EFFECT_HOOK_NAMES = new Set(["useEffect", "useLayoutEffect"]);
 export const HOOKS_WITH_DEPS = new Set(["useEffect", "useLayoutEffect", "useMemo", "useCallback"]);
 
-// Direct CallExpression callees whose function argument is a "sub-handler"
-// — code that runs asynchronously, in response to an event the React render
-// can't observe. Calling a reactive value from inside a sub-handler is the
-// classic case for `useEffectEvent` (see "Separating Events from Effects").
-export const SUB_HANDLER_DIRECT_CALLEE_NAMES = new Set([
+// Direct CallExpression callees that schedule a callback to run later,
+// outside the current render's microtask. Two distinct rules consume this
+// set, so the names below intentionally describe the shape (timers and
+// schedulers) rather than either rule's interpretation.
+//
+// Consumers:
+//   - `prefer-use-effect-event` treats them as "sub-handler" boundaries:
+//     calling a reactive value from inside the scheduled callback is the
+//     classic case for `useEffectEvent` (see "Separating Events from
+//     Effects").
+//   - `no-effect-chain` treats them as external-sync direct callees so a
+//     useEffect that only schedules timers is exempt from the chain rule.
+export const TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES = new Set([
   "setTimeout",
   "setInterval",
   "requestAnimationFrame",
   "requestIdleCallback",
   "queueMicrotask",
 ]);
 
+export const SUB_HANDLER_DIRECT_CALLEE_NAMES = TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES;
+
 // Globals whose values mutate outside the React data flow. Listing
 // them as deps doesn't trigger a re-run when they change because
 // React compares deps with `Object.is` during render — and the read
@@ -417,24 +427,52 @@ export const MUTABLE_GLOBAL_ROOTS = new Set([
   "performance",
 ]);
 
+// Subscription-shaped method names recognized by `prefer-use-sync-external-store`.
+// Covers the canonical `store.subscribe`, the browser `addEventListener` /
+// `addListener`, the EventEmitter `on` / `watch` / `listen`, and shorter
+// store APIs like Jotai's `store.sub`. The detector cares only about the
+// AST shape (one of these is the property name of a MemberExpression
+// callee), never the library that implemented them.
+export const SUBSCRIPTION_METHOD_NAMES = new Set([
+  "subscribe",
+  "addEventListener",
+  "addListener",
+  "on",
+  "watch",
+  "listen",
+  "sub",
+]);
+
+// Methods that pair with the subscription methods above as their cleanup
+// counterparts. Used to recognize a valid `return () => removeEventListener(...)`
+// cleanup form even when the subscribe call is `addEventListener` rather
+// than a `subscribe()` whose return value gets re-bound.
+export const UNSUBSCRIPTION_METHOD_NAMES = new Set([
+  "unsubscribe",
+  "removeEventListener",
+  "removeListener",
+  "off",
+  "unwatch",
+  "unlisten",
+  "unsub",
+]);
+
 // Used by `no-effect-chain` to decide whether an effect is doing
 // "real" external-system synchronization (in which case effects on
 // either side of the chain are exempt, per the article's own caveat
 // about cascading network fetches) versus pure internal reactivity
 // (which is the anti-pattern). A cleanup return is the strongest
 // signal; the curated method list covers the rest.
+//
 // Member-method names that, on their own, mark a call as external
 // sync regardless of receiver. These are unambiguous in real React
 // codebases — they don't clash with built-in JS APIs.
+//
+// Layered on top of `SUBSCRIPTION_METHOD_NAMES` so the subscribe-shape
+// detector and the external-sync detector can never disagree about
+// which method names are "subscriptions."
 export const EXTERNAL_SYNC_MEMBER_METHOD_NAMES = new Set([
-  // Subscriptions / event listeners
-  "subscribe",
-  "addEventListener",
-  "addListener",
-  "on",
-  "watch",
-  "listen",
-  "sub",
+  ...SUBSCRIPTION_METHOD_NAMES,
   // Imperative widget lifecycle (createConnection().connect()/.disconnect())
   "connect",
   "disconnect",
@@ -457,16 +495,15 @@ export const EXTERNAL_SYNC_MEMBER_METHOD_NAMES = new Set([
 // recognized HTTP-client-shaped name. Lets the `axios.get(...)`
 // cascade case work without false-classifying `params.get('id')` as
 // external sync.
+//
+// Layered on top of `FETCH_MEMBER_OBJECTS` (the canonical HTTP-client
+// receiver list used by `containsFetchCall`) so adding a new client
+// name in one place propagates to both detectors.
 export const EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS = new Set([
-  "axios",
-  "ky",
-  "got",
-  "wretch",
-  "ofetch",
+  ...FETCH_MEMBER_OBJECTS,
   "api",
   "client",
   "http",
-  "request",
   "fetcher",
 ]);
 
@@ -477,17 +514,13 @@ export const EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES = new Set([
   "delete",
 ]);
 
+// Direct callees that mark an effect body as external-sync. Combines
+// the shared HTTP-client direct-callee list (`FETCH_CALLEE_NAMES`)
+// with the timer / scheduler list above so all three rule families
+// share a single source of truth for these names.
 export const EXTERNAL_SYNC_DIRECT_CALLEE_NAMES = new Set([
-  "fetch",
-  "ky",
-  "got",
-  "wretch",
-  "ofetch",
-  "setInterval",
-  "setTimeout",
-  "requestAnimationFrame",
-  "requestIdleCallback",
-  "queueMicrotask",
+  ...FETCH_CALLEE_NAMES,
+  ...TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES,
 ]);
 
 export const EXTERNAL_SYNC_OBSERVER_CONSTRUCTORS = new Set([
@@ -497,55 +530,22 @@ export const EXTERNAL_SYNC_OBSERVER_CONSTRUCTORS = new Set([
   "PerformanceObserver",
 ]);
 
-// Subscription-shaped method names recognized by `prefer-use-sync-external-store`.
-// Covers the canonical `store.subscribe`, the browser `addEventListener` /
-// `addListener`, the EventEmitter `on` / `watch` / `listen`, and shorter
-// store APIs like Jotai's `store.sub`. The detector cares only about the
-// AST shape (one of these is the property name of a MemberExpression
-// callee), never the library that implemented them.
-export const SUBSCRIPTION_METHOD_NAMES = new Set([
-  "subscribe",
-  "addEventListener",
-  "addListener",
-  "on",
-  "watch",
-  "listen",
-  "sub",
-]);
-
-// Methods that pair with the subscription methods above as their cleanup
-// counterparts. Used to recognize a valid `return () => removeEventListener(...)`
-// cleanup form even when the subscribe call is `addEventListener` rather
-// than a `subscribe()` whose return value gets re-bound.
-export const UNSUBSCRIPTION_METHOD_NAMES = new Set([
-  "unsubscribe",
-  "removeEventListener",
-  "removeListener",
-  "off",
-  "unwatch",
-  "unlisten",
-  "unsub",
-]);
-
 // Used by `no-event-trigger-state` to recognize when a useEffect body
 // is performing the §6 anti-pattern from "You Might Not Need an Effect"
 // — running an event-shaped side effect (POST, navigation, notification,
 // analytics) that the user actually triggered with a button click.
 // Tightly scoped on purpose — adding a callee name here can produce
 // false positives on pure helper functions, so the bar is "this name
 // 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.
 export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([
+  ...FETCH_CALLEE_NAMES,
   // Network shorthand verbs (article uses `post`)
-  "fetch",
   "post",
   "put",
   "patch",
   "del",
-  // Common HTTP client wrappers
-  "ky",
-  "got",
-  "wretch",
-  "ofetch",
   // Navigation
   "navigate",
   "navigateTo",
@@ -562,20 +562,37 @@ export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([
 ]);
 
 // Recognized when the call shape is `<obj>.<method>(...)` — covers
-// `axios.post`, `api.post`, `router.push`, `analytics.track`,
-// `posthog.capture`, etc. without enumerating every possible object.
+// `axios.post`, `api.post`, `analytics.track`, `posthog.capture`,
+// etc. without enumerating every possible object. Names here are
+// unambiguous: they don't clash with built-in JS prototype methods
+// or common application code.
 export const EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS = new Set([
   "post",
   "put",
   "patch",
   "delete",
-  "push",
-  "replace",
   "navigate",
   "capture",
   "track",
   "logEvent",
 ]);
+
+// HACK: `push` and `replace` are router methods (`router.push("/foo")`,
+// `history.replace("/bar")`) but ALSO universal Array / String prototype
+// methods. `[1, 2].push(3)` and `"a".replace("b", "c")` are NOT event-
+// shaped side effects — calling `setX` after them in a useEffect is
+// usually fine. We only treat them as event-triggered side effects when
+// the receiver looks router-shaped. Keeps the false-positive rate down
+// without losing the `router.push(...)` / `history.replace(...)` cases.
+export const EVENT_TRIGGERED_NAVIGATION_METHOD_NAMES = new Set(["push", "replace"]);
+
+export const NAVIGATION_RECEIVER_NAMES = new Set([
+  "router",
+  "navigation",
+  "navigator",
+  "history",
+  "location",
+]);
 export const CHAINABLE_ITERATION_METHODS = new Set(["map", "filter", "forEach", "flatMap"]);
 export const STORAGE_OBJECTS = new Set(["localStorage", "sessionStorage"]);
 

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

@@ -51,12 +51,32 @@ export const isMemberProperty = (node: EsTreeNode, propertyName: string): boolea
 // `items[0]` → "items". Returns null if the chain bottoms out at
 // anything other than a plain Identifier (e.g. a CallExpression,
 // `this`, etc.). Bare Identifiers also resolve to themselves.
-export const getRootIdentifierName = (node: EsTreeNode | undefined | null): string | null => {
+//
+// When `followCallChains` is true, also walks past the receiver of
+// any intermediate CallExpression — `items.toSorted().filter(fn)` →
+// "items". Off by default because most callers want the receiver of
+// the call (e.g. for "did this assignment write to props?"), not the
+// expression that produced the receiver.
+export const getRootIdentifierName = (
+  node: EsTreeNode | undefined | null,
+  options?: { followCallChains?: boolean },
+): string | null => {
   if (!node) return null;
   if (node.type === "Identifier") return node.name;
+  const followCallChains = options?.followCallChains === true;
   let cursor: EsTreeNode | undefined = node;
-  while (cursor?.type === "MemberExpression") {
-    cursor = cursor.object;
+  while (cursor) {
+    if (cursor.type === "MemberExpression") {
+      cursor = cursor.object;
+      continue;
+    }
+    if (followCallChains && cursor.type === "CallExpression") {
+      const callee = cursor.callee;
+      if (callee?.type !== "MemberExpression") return null;
+      cursor = callee.object;
+      continue;
+    }
+    break;
   }
   return cursor?.type === "Identifier" ? cursor.name : null;
 };

packages/react-doctor/src/plugin/rules/server.ts

@@ -1,5 +1,5 @@
 import { AUTH_CHECK_LOOKAHEAD_STATEMENTS, AUTH_FUNCTION_NAMES } from "../constants.js";
-import { hasDirective, hasUseServerDirective, walkAst } from "../helpers.js";
+import { getRootIdentifierName, hasDirective, hasUseServerDirective, walkAst } from "../helpers.js";
 import type { EsTreeNode, Rule, RuleContext } from "../types.js";
 
 const containsAuthCheck = (statements: EsTreeNode[]): boolean => {
@@ -303,30 +303,13 @@ const callReadsHandlerArgs = (call: EsTreeNode, handlerParamNames: Set<string>):
 
 const DERIVING_ARRAY_METHODS = new Set(["toSorted", "toReversed", "filter", "map", "slice"]);
 
-const getRootIdentifierName = (node: EsTreeNode): string | null => {
-  let cursor: EsTreeNode = node;
-  while (cursor && (cursor.type === "MemberExpression" || cursor.type === "CallExpression")) {
-    if (cursor.type === "MemberExpression") {
-      cursor = cursor.object;
-    } else if (cursor.type === "CallExpression") {
-      const callee = cursor.callee;
-      if (callee?.type === "MemberExpression") {
-        cursor = callee.object;
-      } else {
-        return null;
-      }
-    }
-  }
-  return cursor?.type === "Identifier" ? cursor.name : null;
-};
-
 const expressionDerivesFromIdentifier = (node: EsTreeNode, identifierName: string): boolean => {
   if (node.type !== "CallExpression") return false;
   const callee = node.callee;
   if (callee?.type !== "MemberExpression") return false;
   if (callee.property?.type !== "Identifier") return false;
   if (!DERIVING_ARRAY_METHODS.has(callee.property.name)) return false;
-  return getRootIdentifierName(callee) === identifierName;
+  return getRootIdentifierName(callee, { followCallChains: true }) === identifierName;
 };
 
 // HACK: passing both `<Client list={items} sortedList={items.toSorted()} />`
@@ -350,7 +333,7 @@ export const serverDedupProps: Rule = {
         if (expression.type === "Identifier") {
           identifierAttributes.set(expression.name, attr.name.name);
         } else if (expression.type === "CallExpression") {
-          const root = getRootIdentifierName(expression);
+          const root = getRootIdentifierName(expression, { followCallChains: true });
           if (root && DERIVING_ARRAY_METHODS.has(getDerivingMethodName(expression) ?? "")) {
             if (expressionDerivesFromIdentifier(expression, root)) {
               derivedAttributes.push({ propName: attr.name.name, rootName: root, node: attr });