diff --git a/packages/react-doctor/src/oxlint-config.ts b/packages/react-doctor/src/oxlint-config.ts index 5f0284f0..af6a47d2 100644 --- a/packages/react-doctor/src/oxlint-config.ts +++ b/packages/react-doctor/src/oxlint-config.ts @@ -233,6 +233,7 @@ export const GLOBAL_REACT_DOCTOR_RULES: Record = { "react-doctor/no-cascading-set-state": "warn", "react-doctor/no-effect-event-handler": "warn", "react-doctor/no-effect-event-in-deps": "error", + "react-doctor/no-event-trigger-state": "warn", "react-doctor/no-prop-callback-in-effect": "warn", "react-doctor/no-derived-useState": "warn", "react-doctor/no-direct-state-mutation": "warn", diff --git a/packages/react-doctor/src/plugin/constants.ts b/packages/react-doctor/src/plugin/constants.ts index 9893fd9f..fd43e544 100644 --- a/packages/react-doctor/src/plugin/constants.ts +++ b/packages/react-doctor/src/plugin/constants.ts @@ -419,6 +419,56 @@ export const UNSUBSCRIPTION_METHOD_NAMES = new Set([ "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." +export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([ + // Network shorthand verbs (article uses `post`) + "fetch", + "post", + "put", + "patch", + "del", + // Common HTTP client wrappers + "ky", + "got", + "wretch", + "ofetch", + // Navigation + "navigate", + "navigateTo", + // UI side effects + "showNotification", + "toast", + "alert", + "confirm", + // Analytics + "track", + "logEvent", + "logVisit", + "captureEvent", +]); + +// Recognized when the call shape is `.(...)` — covers +// `axios.post`, `api.post`, `router.push`, `analytics.track`, +// `posthog.capture`, etc. without enumerating every possible object. +export const EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS = new Set([ + "post", + "put", + "patch", + "delete", + "push", + "replace", + "navigate", + "capture", + "track", + "logEvent", +]); 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/index.ts b/packages/react-doctor/src/plugin/index.ts index c93edde9..aed40396 100644 --- a/packages/react-doctor/src/plugin/index.ts +++ b/packages/react-doctor/src/plugin/index.ts @@ -182,6 +182,7 @@ import { noDirectStateMutation, noEffectEventHandler, noEffectEventInDeps, + noEventTriggerState, noFetchInEffect, noPropCallbackInEffect, noSetStateInRender, @@ -204,6 +205,7 @@ const plugin: RulePlugin = { "no-cascading-set-state": noCascadingSetState, "no-effect-event-handler": noEffectEventHandler, "no-effect-event-in-deps": noEffectEventInDeps, + "no-event-trigger-state": noEventTriggerState, "no-prop-callback-in-effect": noPropCallbackInEffect, "no-derived-useState": noDerivedUseState, "no-direct-state-mutation": noDirectStateMutation, 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 4bcaa187..7a02e479 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,8 @@ import { BUILTIN_GLOBAL_NAMESPACE_NAMES, CASCADING_SET_STATE_THRESHOLD, EFFECT_HOOK_NAMES, + EVENT_TRIGGERED_SIDE_EFFECT_CALLEES, + EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS, HOOKS_WITH_DEPS, MUTATING_ARRAY_METHODS, RELATED_USE_STATE_THRESHOLD, @@ -228,45 +230,128 @@ export const noCascadingSetState: Rule = { }; export const noEffectEventHandler: Rule = { - create: (context: RuleContext) => ({ - CallExpression(node: EsTreeNode) { - if (!isHookCall(node, EFFECT_HOOK_NAMES) || (node.arguments?.length ?? 0) < 2) return; - - const callback = getEffectCallback(node); - if (!callback) return; + 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; + }; - const depsNode = node.arguments[1]; - if (depsNode.type !== "ArrayExpression" || !depsNode.elements?.length) return; + 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 dependencyNames = new Set( - depsNode.elements - .filter((element: EsTreeNode) => element?.type === "Identifier") - .map((element: EsTreeNode) => element.name), - ); + const callback = getEffectCallback(node); + if (!callback) return; - const statements = getCallbackStatements(callback); - if (statements.length !== 1) return; + const depsNode = node.arguments[1]; + if (depsNode.type !== "ArrayExpression" || !depsNode.elements?.length) return; - const soleStatement = statements[0]; - if (soleStatement.type !== "IfStatement") return; + const dependencyNames = new Set( + depsNode.elements + .filter((element: EsTreeNode) => element?.type === "Identifier") + .map((element: EsTreeNode) => element.name), + ); - // 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; + 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; + + // 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; + } - context.report({ - node, - message: - "useEffect simulating an event handler — move logic to an actual event handler instead", - }); - }, - }), + context.report({ + node, + message: + "useEffect simulating an event handler — move logic to an actual event handler instead", + }); + }, + }; + }, }; export const noDerivedUseState: Rule = { @@ -1515,3 +1600,199 @@ export const preferUseSyncExternalStore: Rule = { }; }, }; + +// HACK: §6 of "You Might Not Need an Effect" — sending a POST request: +// +// const [jsonToSubmit, setJsonToSubmit] = useState(null); +// useEffect(() => { +// if (jsonToSubmit !== null) { +// post('/api/register', jsonToSubmit); +// } +// }, [jsonToSubmit]); +// +// function handleSubmit(event) { +// event.preventDefault(); +// setJsonToSubmit({ firstName, lastName }); // ← only writer +// } +// +// Detector pre-conditions (all must hold): +// (1) useEffect with deps = [stateX] — single dep that's a useState +// binding declared in this component +// (2) effect body is a single IfStatement guarding on stateX with one +// of: bare truthy, !== null/undefined, === Literal, or .length +// (3) IfStatement.consequent contains a CallExpression whose callee +// is in EVENT_TRIGGERED_SIDE_EFFECT_CALLEES OR a MemberExpression +// whose property is in EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS +// (4) every setStateX call site is inside a JSX `on*` handler (or a +// function bound to one) — i.e. the trigger is set only by user +// interactions, never by other reactive logic +// +// Why all four matter: (1) + (2) recognize the "trigger guard" shape; +// (3) restricts to side effects users would associate with a button +// click; (4) is the strongest signal that the state exists *only* to +// schedule the effect, distinguishing this from §5 (event-shared logic +// triggered by props) which already has its own rule. +// HACK: in JS, `undefined` is parsed as an Identifier (not a Literal +// like `null`). For `x !== undefined`, both sides of the +// BinaryExpression are Identifiers, so a naive "first Identifier +// wins" pick can return `"undefined"` instead of the trigger state +// name — silently dropping the violation for the reversed +// (`undefined !== x`) ordering. Skip the `undefined` / `null` +// sentinel side so the actual state Identifier is what we return. +const SENTINEL_IDENTIFIER_NAMES = new Set(["undefined", "NaN", "null"]); + +const isSentinelIdentifier = (node: EsTreeNode): boolean => + node?.type === "Identifier" && SENTINEL_IDENTIFIER_NAMES.has(node.name); + +const getTriggerGuardRootName = (testNode: EsTreeNode): string | null => { + if (!testNode) return null; + if (testNode.type === "Identifier") return testNode.name; + if (testNode.type === "BinaryExpression") { + if (!["!==", "===", "!=", "=="].includes(testNode.operator)) return null; + for (const side of [testNode.left, testNode.right]) { + if (side?.type === "Identifier" && !isSentinelIdentifier(side)) { + return side.name; + } + } + return null; + } + if ( + testNode.type === "MemberExpression" && + testNode.property?.type === "Identifier" && + testNode.property.name === "length" + ) { + if (testNode.object?.type === "Identifier") return testNode.object.name; + } + if (testNode.type === "UnaryExpression" && testNode.operator === "!") { + return getTriggerGuardRootName(testNode.argument); + } + return null; +}; + +const findTriggeredSideEffectCalleeName = (consequentNode: EsTreeNode): string | null => { + let foundCalleeName: string | null = null; + walkAst(consequentNode, (child: EsTreeNode) => { + if (foundCalleeName) return false; + if (child.type !== "CallExpression") return; + const callee = child.callee; + if (callee?.type === "Identifier" && EVENT_TRIGGERED_SIDE_EFFECT_CALLEES.has(callee.name)) { + foundCalleeName = callee.name; + return; + } + if ( + callee?.type === "MemberExpression" && + callee.property?.type === "Identifier" && + EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS.has(callee.property.name) + ) { + 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; + } + }); + return foundCalleeName; +}; + +const collectHandlerOnlyWriteStateNames = ( + componentBody: EsTreeNode, + useStateBindings: Array<{ valueName: string; setterName: string; declarator: EsTreeNode }>, + handlerBindingNames: Set, +): Set => { + const handlerOnlyWriteStateNames = new Set(); + for (const binding of useStateBindings) { + let didFindAnySetterCall = false; + let areAllSetterCallsInHandlers = true; + walkAst(componentBody, (child: EsTreeNode) => { + if (!areAllSetterCallsInHandlers) return false; + if (child.type !== "CallExpression") return; + if (child.callee?.type !== "Identifier") return; + if (child.callee.name !== binding.setterName) return; + didFindAnySetterCall = true; + if (!isInsideEventHandler(child, handlerBindingNames)) { + areAllSetterCallsInHandlers = false; + } + }); + if (didFindAnySetterCall && areAllSetterCallsInHandlers) { + handlerOnlyWriteStateNames.add(binding.valueName); + } + } + return handlerOnlyWriteStateNames; +}; + +export const noEventTriggerState: Rule = { + create: (context: RuleContext) => { + const checkComponent = (componentBody: EsTreeNode | null | undefined): void => { + if (!componentBody || componentBody.type !== "BlockStatement") return; + + const useStateBindings = collectUseStateBindings(componentBody); + if (useStateBindings.length === 0) return; + + const handlerBindingNames = collectHandlerBindingNames(componentBody); + const handlerOnlyWriteStateNames = collectHandlerOnlyWriteStateNames( + componentBody, + useStateBindings, + handlerBindingNames, + ); + if (handlerOnlyWriteStateNames.size === 0) return; + + // HACK: a state read in render (e.g. ``) + // is dual-purpose — it controls UI AND triggers the effect. + // Calling it "exists only to schedule the effect" is wrong; the + // user can't just delete the state. Reuse the same render- + // reachability machinery that `rerenderStateOnlyInHandlers` + // uses to filter these out (transitive dep graph + walk from + // return expressions). + const returnExpressions = collectReturnExpressions(componentBody); + const dependencyGraph = buildLocalDependencyGraph(componentBody); + const directRenderNames = collectRenderReachableNames(returnExpressions); + const renderReachableNames = expandTransitiveDependencies(directRenderNames, dependencyGraph); + + walkAst(componentBody, (effectCall: EsTreeNode) => { + if (effectCall.type !== "CallExpression") return; + if (!isHookCall(effectCall, EFFECT_HOOK_NAMES)) return; + if ((effectCall.arguments?.length ?? 0) < 2) return; + + const depsNode = effectCall.arguments[1]; + if (depsNode.type !== "ArrayExpression") return; + if ((depsNode.elements?.length ?? 0) !== 1) return; + + const depElement = depsNode.elements[0]; + if (depElement?.type !== "Identifier") return; + if (!handlerOnlyWriteStateNames.has(depElement.name)) return; + // Dual-purpose state — used in render too. Don't claim it + // "exists only to schedule" the effect. + if (renderReachableNames.has(depElement.name)) return; + + const callback = getEffectCallback(effectCall); + if (!callback) return; + + const bodyStatements = getCallbackStatements(callback); + if (bodyStatements.length !== 1) return; + const soleStatement = bodyStatements[0]; + if (soleStatement.type !== "IfStatement") return; + + const guardRootName = getTriggerGuardRootName(soleStatement.test); + if (guardRootName !== depElement.name) return; + + const sideEffectCalleeName = findTriggeredSideEffectCalleeName(soleStatement.consequent); + if (!sideEffectCalleeName) return; + + context.report({ + node: effectCall, + message: `useState "${depElement.name}" exists only to schedule "${sideEffectCalleeName}(...)" from a useEffect — call "${sideEffectCalleeName}(...)" directly inside the event handler that sets it, and delete the state`, + }); + }); + }; + + return { + FunctionDeclaration(node: EsTreeNode) { + if (!node.id?.name || !isUppercaseName(node.id.name)) return; + checkComponent(node.body); + }, + VariableDeclarator(node: EsTreeNode) { + if (!isComponentAssignment(node)) return; + checkComponent(node.init?.body); + }, + }; + }, +}; diff --git a/packages/react-doctor/src/utils/run-oxlint.ts b/packages/react-doctor/src/utils/run-oxlint.ts index bb0b1539..b297b197 100644 --- a/packages/react-doctor/src/utils/run-oxlint.ts +++ b/packages/react-doctor/src/utils/run-oxlint.ts @@ -48,6 +48,7 @@ const RULE_CATEGORY_MAP: Record = { "react-doctor/no-cascading-set-state": "State & Effects", "react-doctor/no-effect-event-handler": "State & Effects", "react-doctor/no-effect-event-in-deps": "State & Effects", + "react-doctor/no-event-trigger-state": "State & Effects", "react-doctor/no-prop-callback-in-effect": "State & Effects", "react-doctor/no-derived-useState": "State & Effects", "react-doctor/no-direct-state-mutation": "State & Effects", @@ -243,6 +244,8 @@ const RULE_HELP_MAP: Record = { "Combine into useReducer: `const [state, dispatch] = useReducer(reducer, initialState)`", "no-effect-event-handler": "Move the conditional logic into onClick, onChange, or onSubmit handlers directly", + "no-event-trigger-state": + "Delete the trigger state (`useState(null)` plus the `useEffect` that watches it) and call the side-effect (`post(...)` / `navigate(...)` / `track(...)`) directly inside the event handler that previously called the setter. State should not exist purely to schedule effect runs", "no-derived-useState": "Remove useState and compute the value inline: `const value = transform(propName)`", "no-direct-state-mutation": diff --git a/packages/react-doctor/tests/fixtures/basic-react/src/state-issues.tsx b/packages/react-doctor/tests/fixtures/basic-react/src/state-issues.tsx index 5f19a1d8..621b9901 100644 --- a/packages/react-doctor/tests/fixtures/basic-react/src/state-issues.tsx +++ b/packages/react-doctor/tests/fixtures/basic-react/src/state-issues.tsx @@ -151,6 +151,28 @@ const SubscribeStorePatternComponent = () => { return
{snapshot}
; }; +declare const post: (url: string, body: unknown) => void; + +const EventTriggerStateComponent = () => { + const [firstName, setFirstName] = useState(""); + const [jsonToSubmit, setJsonToSubmit] = useState<{ firstName: string } | null>(null); + useEffect(() => { + if (jsonToSubmit !== null) { + post("/api/register", jsonToSubmit); + } + }, [jsonToSubmit]); + return ( +
{ + event.preventDefault(); + setJsonToSubmit({ firstName }); + }} + > + setFirstName(event.target.value)} /> +
+ ); +}; + const UncontrolledInputComponent = () => { // HACK: explicit `` keeps TypeScript happy while the // RUNTIME initializer stays undefined — that's what trips the @@ -186,5 +208,6 @@ export { SetStateInRenderComponent, ConditionalSetStateInRenderComponent, SubscribeStorePatternComponent, + EventTriggerStateComponent, UncontrolledInputComponent, }; diff --git a/packages/react-doctor/tests/regressions/state-rules.test.ts b/packages/react-doctor/tests/regressions/state-rules.test.ts index d4dc6900..65528cf2 100644 --- a/packages/react-doctor/tests/regressions/state-rules.test.ts +++ b/packages/react-doctor/tests/regressions/state-rules.test.ts @@ -642,6 +642,323 @@ export const Leaky = () => { }); }); +describe("no-event-trigger-state", () => { + it("flags the article §6 `if (jsonToSubmit !== null) post(...)` POST-trigger shape", async () => { + // https://react.dev/learn/you-might-not-need-an-effect#sending-a-post-request + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-post", { + files: { + "src/Form.tsx": `import { useEffect, useState } from "react"; + +declare const post: (url: string, body: unknown) => void; + +export const Form = () => { + const [firstName, setFirstName] = useState(""); + const [jsonToSubmit, setJsonToSubmit] = useState<{ firstName: string } | null>(null); + useEffect(() => { + if (jsonToSubmit !== null) { + post("/api/register", jsonToSubmit); + } + }, [jsonToSubmit]); + return ( +
{ + event.preventDefault(); + setJsonToSubmit({ firstName }); + }} + > + setFirstName(event.target.value)} /> +
+ ); +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("jsonToSubmit"); + expect(hits[0].message).toContain("post"); + }); + + it("flags `axios.post` member call inside the trigger guard", async () => { + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-axios", { + files: { + "src/Submit.tsx": `import { useEffect, useState } from "react"; + +declare const axios: { post: (url: string, body: unknown) => void }; + +export const Submit = () => { + const [pendingPayload, setPendingPayload] = useState<{ id: number } | null>(null); + useEffect(() => { + if (pendingPayload !== null) { + axios.post("/api/submit", pendingPayload); + } + }, [pendingPayload]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("axios.post"); + }); + + it("flags the bare-truthy guard with `navigate(...)` in the body", async () => { + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-navigate", { + files: { + "src/Wizard.tsx": `import { useEffect, useState } from "react"; + +declare const navigate: (path: string) => void; + +export const Wizard = () => { + const [destination, setDestination] = useState(null); + useEffect(() => { + if (destination) { + navigate(destination); + } + }, [destination]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("navigate"); + }); + + it("does NOT flag the article's GOOD analytics-on-mount example", async () => { + // https://react.dev/learn/you-might-not-need-an-effect#sending-a-post-request + // The mount-time analytics POST is a legitimate effect — empty deps, + // no trigger state, runs once because the form was displayed. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-analytics", { + files: { + "src/AnalyticsForm.tsx": `import { useEffect, useState } from "react"; + +declare const post: (url: string, body: unknown) => void; + +export const AnalyticsForm = () => { + const [firstName, setFirstName] = useState(""); + useEffect(() => { + post("/analytics/event", { eventName: "visit_form" }); + }, []); + return setFirstName(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(0); + }); + + it("does NOT flag when the trigger state is also written outside event handlers", async () => { + // If the state is also set by other reactive logic (another effect, + // top-of-render adjustment), it's not "purely a trigger" — the user + // may have legitimate reasons to re-react when it changes. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-non-handler-write", { + files: { + "src/Mixed.tsx": `import { useEffect, useState } from "react"; + +declare const post: (url: string, body: unknown) => void; + +export const Mixed = ({ initial }: { initial: { id: number } | null }) => { + const [payload, setPayload] = useState<{ id: number } | null>(null); + useEffect(() => { + setPayload(initial); + }, [initial]); + useEffect(() => { + if (payload !== null) { + post("/api/sync", payload); + } + }, [payload]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(0); + }); + + it("does NOT flag when the consequent has no recognized side-effect", async () => { + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-no-side-effect", { + files: { + "src/Computed.tsx": `import { useEffect, useState } from "react"; + +declare const compute: (value: number) => number; + +export const Computed = () => { + const [seed, setSeed] = useState(null); + useEffect(() => { + if (seed !== null) { + compute(seed); + } + }, [seed]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(0); + }); + + it("flags `undefined !== state` (reversed sentinel ordering, Bugbot #155 round 2)", async () => { + // Regression: \`undefined\` is parsed as Identifier, not Literal. + // Naive "first Identifier wins" picked \`"undefined"\` for + // reversed-ordering BinaryExpressions and silently dropped the + // violation. Prefer the non-sentinel side. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-reversed-undefined", { + files: { + "src/Submit.tsx": `import { useEffect, useState } from "react"; + +declare const post: (url: string, body: unknown) => void; + +export const Submit = () => { + const [pendingPayload, setPendingPayload] = useState<{ id: number } | null>(null); + useEffect(() => { + if (undefined !== pendingPayload) { + post("/api/submit", pendingPayload); + } + }, [pendingPayload]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits.length).toBeGreaterThanOrEqual(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 + // value, but no-event-trigger-state has a tighter side-effect- + // callee allowlist. \`customAction()\` isn't in the allowlist, so + // no-event-trigger-state would NOT fire — and the round-2 + // version then silently dropped the warning. Now no-effect- + // event-handler fires unless BOTH predicates match. + const projectDir = setupReactProject( + tempRoot, + "no-event-trigger-state-no-overshadow-on-custom-callee", + { + files: { + "src/Custom.tsx": `import { useEffect, useState } from "react"; + +declare const customAction: () => void; + +export const Custom = () => { + const [trigger, setTrigger] = useState(false); + useEffect(() => { + if (trigger) { + customAction(); + } + }, [trigger]); + return ; +}; +`, + }, + }, + ); + + const handlerHits = await collectRuleHits(projectDir, "no-effect-event-handler"); + const triggerHits = await collectRuleHits(projectDir, "no-event-trigger-state"); + // customAction isn't in the side-effect allowlist → no-event- + // trigger-state stays silent. no-effect-event-handler MUST still + // warn (otherwise we silently dropped the diagnostic). + expect(handlerHits.length).toBe(1); + 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", { + files: { + "src/Wizard.tsx": `import { useEffect, useState } from "react"; + +declare const navigate: (path: string) => void; + +export const Wizard = () => { + const [destination, setDestination] = useState(null); + useEffect(() => { + if (destination) { + navigate(destination); + } + }, [destination]); + return ; +}; +`, + }, + }); + + 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); + }); + + it("does NOT flag dual-purpose state that's also read in render (Bugbot #155)", async () => { + // Regression: \`query\` is BOTH the controlled-input value AND the + // effect trigger. We can't tell the user to "delete the state" + // because the input depends on it. Render-reachability check + // skips this case. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-render-reachable", { + files: { + "src/Search.tsx": `import { useEffect, useState } from "react"; + +declare const track: (eventName: string, payload: string) => void; + +export const Search = () => { + const [query, setQuery] = useState(""); + useEffect(() => { + if (query) { + track("search", query); + } + }, [query]); + return setQuery(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(0); + }); + + it("does NOT flag when the dep is a prop (no local setter at all)", async () => { + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-prop-dep", { + files: { + "src/Notify.tsx": `import { useEffect } from "react"; + +declare const showNotification: (message: string) => void; + +export const Notify = ({ message }: { message: string | null }) => { + useEffect(() => { + if (message !== null) { + showNotification(message); + } + }, [message]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(hits).toHaveLength(0); + }); +}); + describe("no-uncontrolled-input", () => { it("flags `value` without onChange / readOnly", async () => { const projectDir = setupReactProject(tempRoot, "no-uncontrolled-input-no-onchange", { diff --git a/packages/react-doctor/tests/run-oxlint.test.ts b/packages/react-doctor/tests/run-oxlint.test.ts index 6e73a2b9..f4f0538c 100644 --- a/packages/react-doctor/tests/run-oxlint.test.ts +++ b/packages/react-doctor/tests/run-oxlint.test.ts @@ -176,6 +176,12 @@ describe("runOxlint", () => { severity: "warning", category: "State & Effects", }, + "no-event-trigger-state": { + fixture: "state-issues.tsx", + ruleSource: "rules/state-and-effects.ts", + severity: "warning", + category: "State & Effects", + }, }, () => basicReactDiagnostics, );