diff --git a/packages/react-doctor/src/plugin/constants.ts b/packages/react-doctor/src/plugin/constants.ts index 07c22390..d29c7c2a 100644 --- a/packages/react-doctor/src/plugin/constants.ts +++ b/packages/react-doctor/src/plugin/constants.ts @@ -390,11 +390,19 @@ 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", @@ -402,6 +410,8 @@ export const SUB_HANDLER_DIRECT_CALLEE_NAMES = new Set([ "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,36 +530,6 @@ 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, @@ -534,18 +537,15 @@ export const UNSUBSCRIPTION_METHOD_NAMES = new Set([ // 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 `.(...)` — 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"]); diff --git a/packages/react-doctor/src/plugin/helpers.ts b/packages/react-doctor/src/plugin/helpers.ts index e3193e34..110f4f63 100644 --- a/packages/react-doctor/src/plugin/helpers.ts +++ b/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; }; diff --git a/packages/react-doctor/src/plugin/rules/server.ts b/packages/react-doctor/src/plugin/rules/server.ts index 35e96cbd..2cc8e81c 100644 --- a/packages/react-doctor/src/plugin/rules/server.ts +++ b/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): 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 `` @@ -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 }); diff --git a/packages/react-doctor/src/plugin/rules/state-and-effects.ts b/packages/react-doctor/src/plugin/rules/state-and-effects.ts index df110696..2e6105d2 100644 --- a/packages/react-doctor/src/plugin/rules/state-and-effects.ts +++ b/packages/react-doctor/src/plugin/rules/state-and-effects.ts @@ -2,6 +2,7 @@ import { BUILTIN_GLOBAL_NAMESPACE_NAMES, CASCADING_SET_STATE_THRESHOLD, EFFECT_HOOK_NAMES, + EVENT_TRIGGERED_NAVIGATION_METHOD_NAMES, EVENT_TRIGGERED_SIDE_EFFECT_CALLEES, EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS, EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES, @@ -12,6 +13,7 @@ import { HOOKS_WITH_DEPS, MUTABLE_GLOBAL_ROOTS, MUTATING_ARRAY_METHODS, + NAVIGATION_RECEIVER_NAMES, RELATED_USE_STATE_THRESHOLD, SUB_HANDLER_DIRECT_CALLEE_NAMES, SUBSCRIPTION_METHOD_NAMES, @@ -246,128 +248,58 @@ export const noCascadingSetState: Rule = { }; export const noEffectEventHandler: Rule = { - create: (context: RuleContext) => { - // HACK: track per-component useState value names so we can defer - // to noEventTriggerState when the trigger guard is a state-typed - // dep AND the consequent matches the side-effect-callee allowlist. - // Without the second predicate the deference silently drops - // warnings on `if (trigger) customAction()` shapes where the - // callee isn't in noEventTriggerState's allowlist. - const useStateValueNamesStack: Array> = []; - const collectUseStateValueNames = (componentBody: EsTreeNode): Set => { - const stateNames = new Set(); - if (componentBody?.type !== "BlockStatement") return stateNames; - for (const statement of componentBody.body ?? []) { - if (statement.type !== "VariableDeclaration") continue; - for (const declarator of statement.declarations ?? []) { - if (declarator.id?.type !== "ArrayPattern") continue; - if (declarator.init?.type !== "CallExpression") continue; - if (!isHookCall(declarator.init, "useState")) continue; - const valueElement = declarator.id.elements?.[0]; - if (valueElement?.type === "Identifier") stateNames.add(valueElement.name); - } - } - return stateNames; - }; - const isStateValueName = (name: string): boolean => { - for (let frameIndex = useStateValueNamesStack.length - 1; frameIndex >= 0; frameIndex--) { - if (useStateValueNamesStack[frameIndex].has(name)) return true; - } - return false; - }; - - return { - FunctionDeclaration(node: EsTreeNode) { - if (!node.id?.name || !isUppercaseName(node.id.name)) { - useStateValueNamesStack.push(new Set()); - return; - } - useStateValueNamesStack.push(collectUseStateValueNames(node.body)); - }, - "FunctionDeclaration:exit"() { - useStateValueNamesStack.pop(); - }, - VariableDeclarator(node: EsTreeNode) { - if (isComponentAssignment(node)) { - useStateValueNamesStack.push(collectUseStateValueNames(node.init?.body)); - return; - } - if ( - node.init?.type === "ArrowFunctionExpression" || - node.init?.type === "FunctionExpression" - ) { - useStateValueNamesStack.push(new Set()); - } - }, - "VariableDeclarator:exit"(node: EsTreeNode) { - if (isComponentAssignment(node)) { - useStateValueNamesStack.pop(); - return; - } - if ( - node.init?.type === "ArrowFunctionExpression" || - node.init?.type === "FunctionExpression" - ) { - useStateValueNamesStack.pop(); - } - }, - CallExpression(node: EsTreeNode) { - if (!isHookCall(node, EFFECT_HOOK_NAMES) || (node.arguments?.length ?? 0) < 2) return; - - const callback = getEffectCallback(node); - if (!callback) return; - - const depsNode = node.arguments[1]; - if (depsNode.type !== "ArrayExpression" || !depsNode.elements?.length) return; - - const dependencyNames = new Set( - depsNode.elements - .filter((element: EsTreeNode) => element?.type === "Identifier") - .map((element: EsTreeNode) => element.name), - ); + create: (context: RuleContext) => ({ + CallExpression(node: EsTreeNode) { + if (!isHookCall(node, EFFECT_HOOK_NAMES) || (node.arguments?.length ?? 0) < 2) return; - const statements = getCallbackStatements(callback); - if (statements.length !== 1) return; + const callback = getEffectCallback(node); + if (!callback) return; - const soleStatement = statements[0]; - if (soleStatement.type !== "IfStatement") return; + const depsNode = node.arguments[1]; + if (depsNode.type !== "ArrayExpression" || !depsNode.elements?.length) return; - // HACK: §5 of "You Might Not Need an Effect" uses - // `if (product.isInCart)` — a MemberExpression, not a bare - // Identifier. The earlier detector hard-required `Identifier` - // and missed the article's literal example. Walk the test - // down to its root identifier so both shapes match: - // if (isOpen) → root = "isOpen" - // if (product.isInCart) → root = "product" - const rootIdentifierName = getRootIdentifierName(soleStatement.test); - if (!rootIdentifierName || !dependencyNames.has(rootIdentifierName)) return; - - // Defer to noEventTriggerState ONLY when its diagnostic - // would actually fire. Its narrower preconditions (single - // dep, side-effect-callee allowlist) mean a deference based - // only on isStateValueName silently drops warnings for - // shapes like `if (trigger) customAction();` where neither - // rule then reports. Match noEventTriggerState's full set - // here: state-typed dep + recognized side-effect callee in - // the consequent. - // Reuse noEventTriggerState's helper instead of duplicating - // the AST walk + constant lookups; "function would fire" - // ↔ "callee was found in the consequent". - if ( - isStateValueName(rootIdentifierName) && - findTriggeredSideEffectCalleeName(soleStatement.consequent) !== null - ) { - return; - } + const dependencyNames = new Set( + depsNode.elements + .filter((element: EsTreeNode) => element?.type === "Identifier") + .map((element: EsTreeNode) => element.name), + ); - context.report({ - node, - message: - "useEffect simulating an event handler — move logic to an actual event handler instead", - }); - }, - }; - }, + const statements = getCallbackStatements(callback); + if (statements.length !== 1) return; + + const soleStatement = statements[0]; + if (soleStatement.type !== "IfStatement") return; + + // HACK: §5 of "You Might Not Need an Effect" uses + // `if (product.isInCart)` — a MemberExpression, not a bare + // Identifier. The earlier detector hard-required `Identifier` + // and missed the article's literal example. Walk the test + // down to its root identifier so both shapes match: + // if (isOpen) → root = "isOpen" + // if (product.isInCart) → root = "product" + const rootIdentifierName = getRootIdentifierName(soleStatement.test); + if (!rootIdentifierName || !dependencyNames.has(rootIdentifierName)) return; + + // Don't defer to `noEventTriggerState` here. The previous + // implementation tried to ("if the body looks event-shaped, + // let the more specific rule report"), but that deference + // could silently drop diagnostics: `noEventTriggerState` + // requires several preconditions this visitor can't cheaply + // verify (single dep, handler-only writes for that state, + // and not render-reachable). When any of those failed, the + // narrow rule didn't fire AND this rule deferred, so the + // user got nothing. Both rules fire independently — the two + // messages frame the same code differently ("this useEffect + // simulates a handler" vs "this state exists only to schedule + // X from an effect") and a duplicate diagnostic is strictly + // better than a silent drop. + context.report({ + node, + message: + "useEffect simulating an event handler — move logic to an actual event handler instead", + }); + }, + }), }; export const noDerivedUseState: Rule = { @@ -439,13 +371,7 @@ export const noDerivedUseState: Rule = { } if (initializer.type === "MemberExpression" && !initializer.computed) { - let rootIdentifierName: string | null = null; - let cursor: EsTreeNode = initializer; - while (cursor?.type === "MemberExpression") { - cursor = cursor.object; - } - if (cursor?.type === "Identifier") rootIdentifierName = cursor.name; - + const rootIdentifierName = getRootIdentifierName(initializer); if (rootIdentifierName && isPropName(rootIdentifierName)) { context.report({ node, @@ -1738,15 +1664,18 @@ const findTriggeredSideEffectCalleeName = (consequentNode: EsTreeNode): string | foundCalleeName = callee.name; return; } - if ( - callee?.type === "MemberExpression" && - callee.property?.type === "Identifier" && - EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS.has(callee.property.name) - ) { + if (callee?.type === "MemberExpression" && callee.property?.type === "Identifier") { + const propertyName = callee.property.name; + const isUnambiguousMethod = EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS.has(propertyName); + const isNavigationMethod = EVENT_TRIGGERED_NAVIGATION_METHOD_NAMES.has(propertyName); + if (!isUnambiguousMethod && !isNavigationMethod) return; let cursor: EsTreeNode | undefined = callee; while (cursor?.type === "MemberExpression") cursor = cursor.object; const rootName = cursor?.type === "Identifier" ? cursor.name : null; - foundCalleeName = rootName ? `${rootName}.${callee.property.name}` : callee.property.name; + if (isNavigationMethod && (rootName === null || !NAVIGATION_RECEIVER_NAMES.has(rootName))) { + return; + } + foundCalleeName = rootName ? `${rootName}.${propertyName}` : propertyName; } }); return foundCalleeName; @@ -1925,16 +1854,14 @@ const collectWrittenStateNamesInEffect = ( }; const isExternalSyncEffect = (effectCallback: EsTreeNode): boolean => { + // A cleanup return is the strongest signal that the effect owns + // an external resource — once we see one, we don't need to inspect + // the body for an external-sync call shape. if (effectCallback.body?.type === "BlockStatement") { const statements = effectCallback.body.body ?? []; for (const statement of statements) { if (statement.type === "ReturnStatement" && statement.argument) return true; } - } else if (effectCallback.body?.type !== "BlockStatement") { - // Concise arrow body — `useEffect(() => something())`. If the - // expression itself is an external sync call, the effect is - // single-statement-external; otherwise it can't be a chain link - // anyway because chains require setter calls in the body. } let didFindExternalCall = false; @@ -1984,13 +1911,10 @@ const isExternalSyncEffect = (effectCallback: EsTreeNode): boolean => { // etc.). Only count them when the receiver looks like an HTTP // client. if (EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES.has(propertyName)) { - let receiverCursor: EsTreeNode | undefined = child.callee.object; - while (receiverCursor?.type === "MemberExpression") { - receiverCursor = receiverCursor.object; - } + const receiverRootName = getRootIdentifierName(child.callee.object); if ( - receiverCursor?.type === "Identifier" && - EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS.has(receiverCursor.name) + receiverRootName !== null && + EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS.has(receiverRootName) ) { didFindExternalCall = true; } diff --git a/packages/react-doctor/tests/regressions/state-rules.test.ts b/packages/react-doctor/tests/regressions/state-rules.test.ts index 276a7d78..bd7c49bc 100644 --- a/packages/react-doctor/tests/regressions/state-rules.test.ts +++ b/packages/react-doctor/tests/regressions/state-rules.test.ts @@ -839,6 +839,57 @@ export const Submit = () => { expect(hits.length).toBeGreaterThanOrEqual(1); }); + it("does NOT misclassify Array.prototype.push as an event-triggered side effect", async () => { + // Regression: \`push\` is BOTH a router method (router.push("/foo")) + // AND a built-in Array method ([1,2].push(3)). The receiver gates + // the diagnostic — only router-shaped receivers count. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-array-push", { + files: { + "src/Logger.tsx": `import { useEffect, useState } from "react"; + +const auditLog: string[] = []; + +export const Logger = () => { + const [event, setEvent] = useState(null); + useEffect(() => { + if (event) { + auditLog.push(event); + } + }, [event]); + return ; +}; +`, + }, + }); + + const triggerHits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(triggerHits.length).toBe(0); + }); + + it("DOES still flag router.push as an event-triggered side effect", async () => { + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-router-push", { + files: { + "src/Wizard.tsx": `import { useEffect, useState } from "react"; + +declare const router: { push: (path: string) => void }; + +export const Wizard = () => { + const [destination, setDestination] = useState(null); + useEffect(() => { + if (destination) { + router.push(destination); + } + }, [destination]); + return ; +}; +`, + }, + }); + + const triggerHits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(triggerHits.length).toBe(1); + }); + it("KEEPS no-effect-event-handler warning when state-typed dep has a non-allowlisted callee (Bugbot #155 round 3)", async () => { // Regression: round-2 deference was too eager — it skipped // no-effect-event-handler whenever the trigger was a useState @@ -879,12 +930,18 @@ export const Custom = () => { expect(triggerHits.length).toBe(0); }); - it("does NOT double-warn with no-effect-event-handler on the bare-truthy state shape (Bugbot #155 round 2)", async () => { - // Regression: \`if (destination) navigate(destination)\` previously - // triggered BOTH no-effect-event-handler and no-event-trigger-state. - // The former now defers to the latter when the dep is a useState - // value. - const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-no-double-warn", { + it("intentionally double-warns on the bare-truthy state shape (handler + trigger-state both fire)", async () => { + // Regression: \`if (destination) navigate(destination)\` triggers + // BOTH no-effect-event-handler and no-event-trigger-state. An + // earlier implementation tried to defer the former to the latter, + // but that deference silently dropped diagnostics whenever the + // narrower rule's preconditions (handler-only writes, + // not render-reachable, etc.) didn't hold. Both rules now fire + // independently — the messages frame the same code differently + // ("this useEffect simulates a handler" vs "this state exists + // only to schedule navigate from an effect") so a duplicate is + // strictly better than a silent drop. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-double-warn", { files: { "src/Wizard.tsx": `import { useEffect, useState } from "react"; @@ -906,7 +963,7 @@ export const Wizard = () => { const triggerHits = await collectRuleHits(projectDir, "no-event-trigger-state"); const handlerHits = await collectRuleHits(projectDir, "no-effect-event-handler"); expect(triggerHits.length).toBe(1); - expect(handlerHits.length).toBe(0); + expect(handlerHits.length).toBe(1); }); it("does NOT flag dual-purpose state that's also read in render (Bugbot #155)", async () => {