From fa72869c6cdec705c82066693c565ff48831be95 Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:22:36 +0300 Subject: [PATCH 1/8] fix: treat Sanity blueprint files as convention entries (#1748) * fix: treat Sanity blueprint files as convention entries Fixes #1747 sanity.blueprint.ts is a Sanity Studio convention file loaded by filename by the Sanity CLI (sanity blueprints deploy), similar to sanity.config.ts and sanity.cli.ts. It was incorrectly reported as unused by react-doctor/unused-file. Added sanity.blueprint.{ts,js} to the alwaysUsed list in FRAMEWORK_PATTERNS for Sanity, with a regression test. Co-authored-by: Skosh * chore: add changeset for Sanity blueprint fix Co-authored-by: Skosh --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh --- .changeset/sanity-blueprint-convention.md | 9 ++++++++ .../src/project-analysis/collect/entries.ts | 2 +- packages/core/tests/project-analysis.test.ts | 22 +++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 .changeset/sanity-blueprint-convention.md diff --git a/.changeset/sanity-blueprint-convention.md b/.changeset/sanity-blueprint-convention.md new file mode 100644 index 0000000000..bdd69c915e --- /dev/null +++ b/.changeset/sanity-blueprint-convention.md @@ -0,0 +1,9 @@ +--- +"@react-doctor/core": patch +--- + +Treat Sanity blueprint files as convention entries + +`sanity.blueprint.ts` is a Sanity Studio convention file loaded by filename by the Sanity CLI (`sanity blueprints deploy`), similar to `sanity.config.ts` and `sanity.cli.ts`. It was incorrectly reported as unused by `react-doctor/unused-file`. + +Fixes #1747 diff --git a/packages/core/src/project-analysis/collect/entries.ts b/packages/core/src/project-analysis/collect/entries.ts index f151140227..464429192c 100644 --- a/packages/core/src/project-analysis/collect/entries.ts +++ b/packages/core/src/project-analysis/collect/entries.ts @@ -2726,7 +2726,7 @@ const FRAMEWORK_PATTERNS: ToolingPluginDefinition[] = [ enablers: ["sanity", "@sanity/cli"], enablerPrefixes: ["@sanity/"], entryPatterns: [], - alwaysUsed: ["sanity.config.{ts,js}", "sanity.cli.{ts,js}"], + alwaysUsed: ["sanity.config.{ts,js}", "sanity.cli.{ts,js}", "sanity.blueprint.{ts,js}"], }, { enablers: ["astro"], diff --git a/packages/core/tests/project-analysis.test.ts b/packages/core/tests/project-analysis.test.ts index 6949c6b0bb..34f7d2bf8e 100644 --- a/packages/core/tests/project-analysis.test.ts +++ b/packages/core/tests/project-analysis.test.ts @@ -638,6 +638,28 @@ describe("analyzeProject", () => { expect(relativePaths(rootDirectory, result.unusedFiles)).toEqual(["src/orphan.jsx"]); }); + it("treats Sanity blueprint configuration as a convention entry", async () => { + const rootDirectory = createProject( + { + "sanity.config.ts": `export default { name: "studio", title: "Studio" };`, + "sanity.blueprint.ts": ` + import { defineBlueprint } from "@sanity/blueprints"; + import { blueprintHelper } from "./lib/blueprint-helper"; + export default defineBlueprint({ + resources: blueprintHelper, + }); + `, + "lib/blueprint-helper.ts": "export const blueprintHelper = [];", + "src/orphan.ts": "export const orphan = true;", + }, + { dependencies: { sanity: "1.0.0", "@sanity/blueprints": "1.0.0" } }, + ); + + const result = await analyzeProject({ rootDirectory }); + + expect(relativePaths(rootDirectory, result.unusedFiles)).toEqual(["src/orphan.ts"]); + }); + it("discovers static entries from CoffeeScript interpolated require factories", async () => { const rootDirectory = createProject( { From 576d7563ffc7a6208cd77d3e3ec81d7f91bf6143 Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:22:39 +0300 Subject: [PATCH 2/8] fix: respect 'use no memo' directive in react-compiler-no-manual-memoization rule (#1750) * fix: respect 'use no memo' directive in react-compiler-no-manual-memoization rule When a component has the 'use no memo' directive, React Compiler skips optimization for that component, so manual memoization (useMemo, useCallback, memo) is still needed. This change adds support for detecting the 'use no memo' directive and suppresses the react-compiler-no-manual-memoization rule in those cases. - Add hasUseNoMemoDirective utility function - Update rule to check for directive in enclosing function (useMemo/useCallback) - Update rule to check for directive in wrapped component (memo) - Add comprehensive tests including regression tests Fixes #1749 Co-authored-by: Skosh * chore: add changeset for use no memo directive fix Co-authored-by: Skosh * fix: respect React Compiler opt-out directives --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh Co-authored-by: Aiden Bai --- .changeset/respect-use-no-memo-directive.md | 9 ++ ...emoization--compiler-opt-out-directive.tsx | 12 ++ ...-no-manual-memoization.regressions.test.ts | 51 ++++++ ...act-compiler-no-manual-memoization.test.ts | 149 ++++++++++++++++++ .../react-compiler-no-manual-memoization.ts | 75 +++++---- .../src/plugin/utils/has-directive.ts | 17 +- .../has-react-compiler-opt-out-directive.ts | 7 + 7 files changed, 290 insertions(+), 30 deletions(-) create mode 100644 .changeset/respect-use-no-memo-directive.md create mode 100644 packages/fuzz/corpus/regressions/react-compiler-no-manual-memoization--compiler-opt-out-directive.tsx create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/utils/has-react-compiler-opt-out-directive.ts diff --git a/.changeset/respect-use-no-memo-directive.md b/.changeset/respect-use-no-memo-directive.md new file mode 100644 index 0000000000..a3764f2243 --- /dev/null +++ b/.changeset/respect-use-no-memo-directive.md @@ -0,0 +1,9 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +fix: respect "use no memo" directive in react-compiler-no-manual-memoization rule + +When a function or module has a React Compiler opt-out directive, the compiler skips optimization, so manual memoization can still be necessary. The rule now respects `"use no memo"`, its `"use no forget"` alias, and local components passed to `memo`. + +Fixes #1749 diff --git a/packages/fuzz/corpus/regressions/react-compiler-no-manual-memoization--compiler-opt-out-directive.tsx b/packages/fuzz/corpus/regressions/react-compiler-no-manual-memoization--compiler-opt-out-directive.tsx new file mode 100644 index 0000000000..a55ef34eea --- /dev/null +++ b/packages/fuzz/corpus/regressions/react-compiler-no-manual-memoization--compiler-opt-out-directive.tsx @@ -0,0 +1,12 @@ +// verdict: pass +// rule: react-compiler-no-manual-memoization +// weakness: framework-gating +// source: GitHub issue #1749 + +import { useMemo } from "react"; + +export const LegacyComponent = () => { + "use no memo"; + const cachedValue = useMemo(() => getValue(), []); + return {cachedValue}; +}; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.regressions.test.ts index 9df3efcd24..614bc6626c 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.regressions.test.ts @@ -168,4 +168,55 @@ export default function ({ onPress }) { ); expect(result.diagnostics).toEqual([]); }); + + it("does not flag useMemo inside a component with 'use no memo' directive (issue #1749)", () => { + const result = run( + `import { useMemo } from "react"; +export function SomeComponent() { + "use no memo"; + const something = useMemo(() => getSomething(), []); + return
{something}
; +}`, + ); + expect(result.diagnostics).toEqual([]); + }); + + it("does not flag useCallback inside a component with 'use no memo' directive", () => { + const result = run( + `import { useCallback } from "react"; +export function LegacyComponent() { + "use no memo"; + const handler = useCallback(() => console.log("click"), []); + return ; +}`, + ); + expect(result.diagnostics).toEqual([]); + }); + + it("does not flag memo wrapping a component with 'use no memo' directive", () => { + const result = run( + `import { memo } from "react"; +const Component = memo(function Inner({ value }) { + "use no memo"; + return {value}; +});`, + ); + expect(result.diagnostics).toEqual([]); + }); + + it("still flags useMemo in a nested component without 'use no memo' inside a parent with the directive", () => { + const result = run( + `import { useMemo } from "react"; +export function OuterComponent() { + "use no memo"; + const InnerComponent = () => { + const cached = useMemo(() => 1, []); + return
{cached}
; + }; + return ; +}`, + ); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0]?.message).toContain("useMemo"); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.test.ts index d0f0fd48a4..cfddec6114 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.test.ts @@ -315,4 +315,153 @@ export default Wrapped;`, 0, ); }); + + it("does not flag `useMemo` in a component with 'use no memo' directive", () => { + expectDiagnosticCount( + `import { useMemo } from "react"; +export function Component() { + "use no memo"; + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; +}`, + 0, + ); + }); + + it("does not flag `useCallback` in a component with 'use no memo' directive", () => { + expectDiagnosticCount( + `import { useCallback } from "react"; +export function Component() { + "use no memo"; + const handler = useCallback(() => undefined, []); + return ; +}`, + 0, + ); + }); + + it("does not flag `React.useMemo` in a component with 'use no memo' directive (namespace import)", () => { + expectDiagnosticCount( + `import * as React from "react"; +export function Component() { + "use no memo"; + const cachedValue = React.useMemo(() => 1, []); + return {cachedValue}; +}`, + 0, + ); + }); + + it("does not flag when 'use no memo' appears in single quotes", () => { + expectDiagnosticCount( + `import { useMemo } from "react"; +export function Component() { + 'use no memo'; + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; +}`, + 0, + ); + }); + + it("still flags `useMemo` in a nested function without 'use no memo' inside a component that has the directive", () => { + expectDiagnosticCount( + `import { useMemo } from "react"; +export function OuterComponent() { + "use no memo"; + const InnerComponent = () => { + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; + }; + return ; +}`, + 1, + ); + }); + + it("does not flag `useMemo` in arrow function component with 'use no memo'", () => { + expectDiagnosticCount( + `import { useMemo } from "react"; +export const Component = () => { + "use no memo"; + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; +};`, + 0, + ); + }); + + it("does not flag manual memoization when the module has a compiler opt-out directive", () => { + expectDiagnosticCount( + `"use no memo"; +import { memo, useMemo } from "react"; +export const Component = memo(function Component() { + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; +});`, + 0, + ); + }); + + it("does not flag manual memoization with the 'use no forget' alias", () => { + expectDiagnosticCount( + `import { useMemo } from "react"; +export function Component() { + "use no forget"; + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; +}`, + 0, + ); + }); + + it("does not flag memo around a local component with a compiler opt-out directive", () => { + expectDiagnosticCount( + `import { memo } from "react"; +function Component() { + "use no memo"; + return Value; +} +export default memo(Component);`, + 0, + ); + }); + + it("still flags manual memoization when the string is outside the directive prologue", () => { + expectDiagnosticCount( + `import { useMemo } from "react"; +export function Component() { + prepare(); + "use no memo"; + const cachedValue = useMemo(() => 1, []); + return {cachedValue}; +}`, + 1, + ); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.ts index af686ced3f..8b6933220a 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/architecture/react-compiler-no-manual-memoization.ts @@ -2,10 +2,12 @@ import { defineRule } from "../../utils/define-rule.js"; import type { EsTreeNode } from "../../utils/es-tree-node.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; import { findEnclosingFunction } from "../../utils/find-enclosing-function.js"; +import { findVariableInitializer } from "../../utils/find-variable-initializer.js"; import { getImportedNameFromModule, isImportedFromModule, } from "../../utils/find-import-source-for-name.js"; +import { hasReactCompilerOptOutDirective } from "../../utils/has-react-compiler-opt-out-directive.js"; import { isCanonicalReactNamespaceName } from "../../utils/is-canonical-react-namespace-name.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; import { isReactComponentOrHookName } from "../../utils/is-react-component-or-hook-name.js"; @@ -147,31 +149,50 @@ export const reactCompilerNoManualMemoization = defineRule({ requires: ["react-compiler"], recommendation: "Profile compiler-managed code and remove `useMemo`, `useCallback`, or `memo` only when the manual cache no longer carries behavioral or performance intent.", - create: (context: RuleContext) => ({ - CallExpression(node: EsTreeNodeOfType<"CallExpression">) { - const apiName = resolveReactApiNameForCallee(node.callee, context); - if (!apiName) return; - // `memo(Component, areEqual)` with a custom comparator encodes - // bespoke equality the compiler can't replicate, so it isn't - // redundant — leave it alone. A nullish second arg is no comparator - // at all, so it doesn't earn the exemption. - if (apiName === "memo") { - const comparatorArgument = node.arguments?.[1]; - if (comparatorArgument && !isNullishComparatorArgument(comparatorArgument)) return; - } else { - // `useMemo` / `useCallback` are only redundant inside a function - // the compiler will actually compile. Inside a function it skips - // (an anonymous arrow handed to a non-React HOC, a non-component - // helper) nothing is auto-cached, so the manual memoization stays. - const enclosingFunction = findEnclosingFunction(node); - if (!enclosingFunction || !isCompilerInferableFunction(enclosingFunction)) return; - } - const removalMessage = REMOVAL_MESSAGE_BY_REACT_API_NAME.get(apiName); - if (!removalMessage) return; - context.report({ - node, - message: removalMessage, - }); - }, - }), + create: (context: RuleContext) => { + let doesModuleOptOutOfReactCompiler = false; + return { + Program(node: EsTreeNodeOfType<"Program">) { + doesModuleOptOutOfReactCompiler = hasReactCompilerOptOutDirective(node); + }, + CallExpression(node: EsTreeNodeOfType<"CallExpression">) { + if (doesModuleOptOutOfReactCompiler) return; + const apiName = resolveReactApiNameForCallee(node.callee, context); + if (!apiName) return; + // `memo(Component, areEqual)` with a custom comparator encodes + // bespoke equality the compiler can't replicate, so it isn't + // redundant — leave it alone. A nullish second arg is no comparator + // at all, so it doesn't earn the exemption. + if (apiName === "memo") { + const comparatorArgument = node.arguments?.[1]; + if (comparatorArgument && !isNullishComparatorArgument(comparatorArgument)) return; + let wrappedComponent = stripParenExpression(node.arguments?.[0]); + if (wrappedComponent && isNodeOfType(wrappedComponent, "Identifier")) { + const componentBinding = findVariableInitializer( + wrappedComponent, + wrappedComponent.name, + ); + if (componentBinding?.initializer) { + wrappedComponent = stripParenExpression(componentBinding.initializer); + } + } + if (wrappedComponent && hasReactCompilerOptOutDirective(wrappedComponent)) return; + } else { + // `useMemo` / `useCallback` are only redundant inside a function + // the compiler will actually compile. Inside a function it skips + // (an anonymous arrow handed to a non-React HOC, a non-component + // helper) nothing is auto-cached, so the manual memoization stays. + const enclosingFunction = findEnclosingFunction(node); + if (!enclosingFunction || !isCompilerInferableFunction(enclosingFunction)) return; + if (hasReactCompilerOptOutDirective(enclosingFunction)) return; + } + const removalMessage = REMOVAL_MESSAGE_BY_REACT_API_NAME.get(apiName); + if (!removalMessage) return; + context.report({ + node, + message: removalMessage, + }); + }, + }; + }, }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-directive.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-directive.ts index 7d37fe5b05..66ded44124 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-directive.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-directive.ts @@ -1,9 +1,20 @@ import type { EsTreeNode } from "./es-tree-node.js"; import { isNodeOfType } from "./is-node-of-type.js"; -export const hasDirective = (programNode: EsTreeNode, directive: string): boolean => { - if (!isNodeOfType(programNode, "Program")) return false; - for (const statement of programNode.body) { +export const hasDirective = (node: EsTreeNode, directive: string): boolean => { + let statements: EsTreeNode[] | null = null; + if (isNodeOfType(node, "Program")) { + statements = node.body; + } else if ( + (isNodeOfType(node, "FunctionDeclaration") || + isNodeOfType(node, "FunctionExpression") || + isNodeOfType(node, "ArrowFunctionExpression")) && + isNodeOfType(node.body, "BlockStatement") + ) { + statements = node.body.body; + } + if (statements === null) return false; + for (const statement of statements) { if (!isNodeOfType(statement, "ExpressionStatement") || statement.directive === undefined) { return false; } diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-react-compiler-opt-out-directive.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-react-compiler-opt-out-directive.ts new file mode 100644 index 0000000000..fd1940e593 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/has-react-compiler-opt-out-directive.ts @@ -0,0 +1,7 @@ +import type { EsTreeNode } from "./es-tree-node.js"; +import { hasDirective } from "./has-directive.js"; + +const REACT_COMPILER_OPT_OUT_DIRECTIVES = new Set(["use no memo", "use no forget"]); + +export const hasReactCompilerOptOutDirective = (node: EsTreeNode): boolean => + [...REACT_COMPILER_OPT_OUT_DIRECTIVES].some((directive) => hasDirective(node, directive)); From ce5ce930de300c7dcddb44819cdecb93de336bfd Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:22:43 +0300 Subject: [PATCH 3/8] fix(cli): increase runtime trace finalization timeout to 60s (#1753) * fix(cli): increase runtime trace finalization timeout to 60s Chrome needs more time to finalize large performance traces, especially for longer recording sessions (up to 5 minutes). The previous 10-second timeout was insufficient for complex React applications generating substantial trace data. Increased timeout from 10s to 60s, which: - Aligns with industry best practices for CDP trace finalization - Accommodates traces from the max 5-minute recording duration - Prevents spurious timeouts on large/complex applications Also improved the error message to be more actionable. Closes #1752 Co-authored-by: Skosh * fix(cli): harden runtime trace finalization --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh Co-authored-by: Aiden Bai --- .changeset/trace-timeout-increase.md | 5 +++ .../src/cli/runtime-scan/constants.ts | 2 +- .../cli/runtime-scan/record-runtime-trace.ts | 18 ++++++++-- .../react-doctor/tests/runtime-scan.test.ts | 36 +++++++++++++++++++ 4 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 .changeset/trace-timeout-increase.md diff --git a/.changeset/trace-timeout-increase.md b/.changeset/trace-timeout-increase.md new file mode 100644 index 0000000000..fe14bb3917 --- /dev/null +++ b/.changeset/trace-timeout-increase.md @@ -0,0 +1,5 @@ +--- +"react-doctor": patch +--- + +Increase runtime trace finalization timeout from 10 to 60 seconds to handle large traces from longer recording sessions diff --git a/packages/react-doctor/src/cli/runtime-scan/constants.ts b/packages/react-doctor/src/cli/runtime-scan/constants.ts index a93b6fd1c1..f806830d93 100644 --- a/packages/react-doctor/src/cli/runtime-scan/constants.ts +++ b/packages/react-doctor/src/cli/runtime-scan/constants.ts @@ -43,7 +43,7 @@ export const RUNTIME_SCAN_BROWSER_HEIGHT_PX = 900; export const RUNTIME_SCAN_DURATION_PRECISION_DIGITS = 1; export const RUNTIME_SCAN_TRACE_FILE_MODE = 0o600; export const RUNTIME_SCAN_TRACE_FILE_EXTENSION = ".json.gz"; -export const RUNTIME_SCAN_TRACING_COMPLETE_TIMEOUT_MS = 10_000; +export const RUNTIME_SCAN_TRACING_COMPLETE_TIMEOUT_MS = 60_000; export const RUNTIME_SCAN_PROBE_RELATIVE_PATH = "runtime-scan/browser-probe.iife.js"; export const RUNTIME_SCAN_PROBE_SNAPSHOT_BINDING_NAME_PLACEHOLDER = "__REACT_DOCTOR_RUNTIME_SCAN_CAPTURE__"; diff --git a/packages/react-doctor/src/cli/runtime-scan/record-runtime-trace.ts b/packages/react-doctor/src/cli/runtime-scan/record-runtime-trace.ts index 641c521cde..37a07f5151 100644 --- a/packages/react-doctor/src/cli/runtime-scan/record-runtime-trace.ts +++ b/packages/react-doctor/src/cli/runtime-scan/record-runtime-trace.ts @@ -158,15 +158,27 @@ const startDevtoolsTrace = async (cdpSession: CDPSession): Promise => { } }; -const endDevtoolsTrace = async (cdpSession: CDPSession): Promise => { +interface DevtoolsTraceSession { + once(event: "Tracing.tracingComplete", listener: (event: { stream?: string }) => void): void; + send(method: "Tracing.end"): Promise; +} + +export const endDevtoolsTrace = async ( + cdpSession: DevtoolsTraceSession, + timeoutMs = RUNTIME_SCAN_TRACING_COMPLETE_TIMEOUT_MS, +): Promise => { const traceComplete = new Promise<{ stream?: string }>((resolve) => { cdpSession.once("Tracing.tracingComplete", resolve); }); let timeoutHandle: ReturnType | undefined; const traceTimeout = new Promise((_resolve, reject) => { timeoutHandle = setTimeout(() => { - reject(new Error("Chrome did not finish the performance trace in time.")); - }, RUNTIME_SCAN_TRACING_COMPLETE_TIMEOUT_MS); + reject( + new CliInputError( + "Chrome did not finalize the performance trace within 60 seconds. Record a shorter interaction and retry.", + ), + ); + }, timeoutMs); }); let stream: string | undefined; try { diff --git a/packages/react-doctor/tests/runtime-scan.test.ts b/packages/react-doctor/tests/runtime-scan.test.ts index 1d7cdce9ae..791c72d6c1 100644 --- a/packages/react-doctor/tests/runtime-scan.test.ts +++ b/packages/react-doctor/tests/runtime-scan.test.ts @@ -7,7 +7,9 @@ import { RUNTIME_SCAN_MAX_LOAF_ENTRIES, RUNTIME_SCAN_MAX_SNAPSHOT_PAYLOAD_BYTES, RUNTIME_SCAN_MAX_STRING_LENGTH, + RUNTIME_SCAN_TRACING_COMPLETE_TIMEOUT_MS, } from "../src/cli/runtime-scan/constants.js"; +import { endDevtoolsTrace } from "../src/cli/runtime-scan/record-runtime-trace.js"; import { formatRuntimeScanReport } from "../src/cli/runtime-scan/format-runtime-scan-report.js"; import { mergeRuntimeScanProbeSnapshots } from "../src/cli/runtime-scan/merge-runtime-scan-probe-snapshots.js"; import { @@ -20,6 +22,8 @@ import { sanitizeRuntimeUrl } from "../src/cli/runtime-scan/sanitize-runtime-url import type { RuntimeScanProbeSnapshot } from "../src/cli/runtime-scan/types.js"; import { scrubRunArguments } from "../src/cli/utils/scrub-run-arguments.js"; +const TEST_TRACE_TIMEOUT_MS = 1; + const snapshot: RuntimeScanProbeSnapshot = { timeOrigin: 1_000, finalUrl: "https://example.com/dashboard?token=secret#private", @@ -348,6 +352,38 @@ describe("runtime scan report", () => { }); describe("runtime scan input", () => { + it("allows 60 seconds for Chrome to finalize a trace", () => { + expect(RUNTIME_SCAN_TRACING_COMPLETE_TIMEOUT_MS).toBe(60_000); + }); + + it("returns the trace stream after Chrome reports completion", async () => { + let completeTrace: ((event: { stream?: string }) => void) | undefined; + const cdpSession = { + once: (_event: "Tracing.tracingComplete", listener: (event: { stream?: string }) => void) => { + completeTrace = listener; + }, + send: async (_method: "Tracing.end") => { + completeTrace?.({ stream: "trace-stream" }); + }, + }; + + await expect(endDevtoolsTrace(cdpSession)).resolves.toBe("trace-stream"); + }); + + it("reports an actionable error when Chrome does not finish a trace", async () => { + const cdpSession = { + once: ( + _event: "Tracing.tracingComplete", + _listener: (event: { stream?: string }) => void, + ) => {}, + send: async (_method: "Tracing.end") => {}, + }; + + await expect(endDevtoolsTrace(cdpSession, TEST_TRACE_TIMEOUT_MS)).rejects.toThrow( + "Record a shorter interaction and retry", + ); + }); + it("requires an explicit URL outside an interactive terminal", async () => { await expect(runtimeScanAction(undefined, { format: "text" })).rejects.toThrow( "A URL is required outside an interactive terminal", From d030b113306c7d3a56426e4ce4c6be46ca20ae1e Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Sat, 5 Sep 2026 14:22:48 -0700 Subject: [PATCH 4/8] fix(core): detect Sentry-wrapped compiler config (#1755) --- .../detect-sentry-wrapped-compiler-config.md | 5 ++ .../react-compiler-config-evaluator.ts | 41 +++++++++++ packages/core/tests/discover-project.test.ts | 69 +++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 .changeset/detect-sentry-wrapped-compiler-config.md diff --git a/.changeset/detect-sentry-wrapped-compiler-config.md b/.changeset/detect-sentry-wrapped-compiler-config.md new file mode 100644 index 0000000000..5df8e37a98 --- /dev/null +++ b/.changeset/detect-sentry-wrapped-compiler-config.md @@ -0,0 +1,5 @@ +--- +"@react-doctor/core": patch +--- + +Detect React Compiler configuration passed through `withSentryConfig`. diff --git a/packages/core/src/project-info/react-compiler-config-evaluator.ts b/packages/core/src/project-info/react-compiler-config-evaluator.ts index c2955fd8f0..3421a0acea 100644 --- a/packages/core/src/project-info/react-compiler-config-evaluator.ts +++ b/packages/core/src/project-info/react-compiler-config-evaluator.ts @@ -1491,6 +1491,17 @@ const analyzeConfigIdentifier = ( ); }; +const hasReactCompilerConfigInSentryWrapperArgument = ( + callExpression: ts.CallExpression, + analysis: ConfigExpressionAnalysis, + moduleSpecifier: string, + exportName: string, +): boolean => { + if (moduleSpecifier !== "@sentry/nextjs" || exportName !== "withSentryConfig") return false; + const [configArgument] = callExpression.arguments; + return Boolean(configArgument && analyzeConfigNode(configArgument, analysis, false)); +}; + const analyzeConfigCallTarget = ( callExpression: ts.CallExpression, analysis: ConfigExpressionAnalysis, @@ -1543,6 +1554,16 @@ const analyzeConfigCallTarget = ( ) { return true; } + if ( + hasReactCompilerConfigInSentryWrapperArgument( + callExpression, + analysis, + requiredModuleSpecifier, + propertyName, + ) + ) { + return true; + } const hasCompilerTransform = analyzeImportedConfig({ analysis, moduleSpecifier: requiredModuleSpecifier, @@ -1570,6 +1591,16 @@ const analyzeConfigCallTarget = ( ) { return true; } + if ( + hasReactCompilerConfigInSentryWrapperArgument( + callExpression, + analysis, + importBinding.moduleSpecifier, + propertyName, + ) + ) { + return true; + } const hasCompilerTransform = analyzeImportedConfig({ analysis, moduleSpecifier: importBinding.moduleSpecifier, @@ -1864,6 +1895,16 @@ const analyzeConfigNode = ( ) { return true; } + if ( + hasReactCompilerConfigInSentryWrapperArgument( + node, + analysis, + importBinding.moduleSpecifier, + importBinding.exportName, + ) + ) { + return true; + } if ( allowCompilerTransform && importBinding.moduleSpecifier === "@rolldown/plugin-babel" && diff --git a/packages/core/tests/discover-project.test.ts b/packages/core/tests/discover-project.test.ts index 26f770868c..605d97a558 100644 --- a/packages/core/tests/discover-project.test.ts +++ b/packages/core/tests/discover-project.test.ts @@ -2704,6 +2704,75 @@ describe("discoverProject", () => { }, ); + it.each([ + { + name: "named-import", + config: + "import { withSentryConfig } from '@sentry/nextjs'; const nextConfig = { reactCompiler: true }; export default withSentryConfig(nextConfig, { org: 'x' });", + helper: null, + expected: true, + }, + { + name: "namespace-import", + config: + "import * as Sentry from '@sentry/nextjs'; const nextConfig = { reactCompiler: true }; export default Sentry.withSentryConfig(nextConfig, { org: 'x' });", + helper: null, + expected: true, + }, + { + name: "commonjs-require", + config: + "const Sentry = require('@sentry/nextjs'); const nextConfig = { reactCompiler: true }; module.exports = Sentry.withSentryConfig(nextConfig, { org: 'x' });", + helper: null, + expected: true, + }, + { + name: "local-wrapper", + config: + "import { wrap } from './wrapper'; const nextConfig = { reactCompiler: true }; export default wrap(nextConfig);", + helper: + "import { withSentryConfig } from '@sentry/nextjs'; export const wrap = (config) => withSentryConfig(config, { org: 'x' });", + expected: true, + }, + { + name: "compiler-only-in-options", + config: + "import { withSentryConfig } from '@sentry/nextjs'; export default withSentryConfig({ reactCompiler: false }, { reactCompiler: true });", + helper: null, + expected: false, + }, + ])( + "detects React Compiler through Sentry config wrappers: $name", + ({ name, config, helper, expected }) => { + const projectDirectory = path.join(tempDirectory, `nextjs-sentry-wrapper-${name}`); + const wrapperDirectory = path.join(projectDirectory, "node_modules", "@sentry", "nextjs"); + fs.mkdirSync(wrapperDirectory, { recursive: true }); + fs.writeFileSync( + path.join(projectDirectory, "package.json"), + JSON.stringify({ + name: `nextjs-sentry-wrapper-${name}`, + dependencies: { next: "^16.0.0", react: "^19.0.0", "@sentry/nextjs": "^10.0.0" }, + }), + ); + fs.writeFileSync( + path.join(wrapperDirectory, "package.json"), + JSON.stringify({ + name: "@sentry/nextjs", + type: "module", + exports: "./index.js", + }), + ); + fs.writeFileSync( + path.join(wrapperDirectory, "index.js"), + "export const withSentryConfig = (_config, _options) => ({ sentry: true });\n", + ); + fs.writeFileSync(path.join(projectDirectory, "next.config.ts"), config); + if (helper) fs.writeFileSync(path.join(projectDirectory, "wrapper.ts"), helper); + + expect(discoverProject(projectDirectory).hasReactCompiler).toBe(expected); + }, + ); + it("detects the Rsbuild React Compiler transform", () => { const projectDirectory = path.join(tempDirectory, "rsbuild-react-compiler"); fs.mkdirSync(projectDirectory, { recursive: true }); From 2e3f6eb98a0eec411b3adf87d8205d3d654538ac Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:28:55 +0300 Subject: [PATCH 5/8] fix(rule): narrow magic-link mutation exemption (#1760) * fix: exempt send/resend/notify/email mutations from cache invalidation requirement These operations are cache-effect-free (sending emails, notifications, codes) with no server data that could go stale. Added send, resend, notify, and email to READ_ONLY_MUTATION_WORDS alongside download, export, validate, etc. Closes #1759 Co-authored-by: Skosh * chore: add changeset for send/resend/notify/email exemption Co-authored-by: Skosh * fix(rule): narrow magic link mutations --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh Co-authored-by: Aiden Bai --- .../send-resend-notify-email-exemption.md | 5 ++ ...sing-invalidation--magic-link-delivery.tsx | 13 +++++ ...n-missing-invalidation.regressions.test.ts | 53 +++++++++++++++++++ .../query-mutation-missing-invalidation.ts | 1 + 4 files changed, 72 insertions(+) create mode 100644 .changeset/send-resend-notify-email-exemption.md create mode 100644 packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--magic-link-delivery.tsx diff --git a/.changeset/send-resend-notify-email-exemption.md b/.changeset/send-resend-notify-email-exemption.md new file mode 100644 index 0000000000..fabd2bbd28 --- /dev/null +++ b/.changeset/send-resend-notify-email-exemption.md @@ -0,0 +1,5 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +Exempt magic-link delivery mutations from `query-mutation-missing-invalidation` while keeping generic send, notification, and email mutations reportable. diff --git a/packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--magic-link-delivery.tsx b/packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--magic-link-delivery.tsx new file mode 100644 index 0000000000..92d44ccdec --- /dev/null +++ b/packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--magic-link-delivery.tsx @@ -0,0 +1,13 @@ +// verdict: pass +// rule: query-mutation-missing-invalidation +// weakness: name-heuristic +// source: GitHub issue #1759 + +import { useMutation } from "@tanstack/react-query"; + +declare const sendMagicLink: (email: string) => Promise; + +export const useSendMagicLink = () => + useMutation({ + mutationFn: sendMagicLink, + }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.regressions.test.ts index db8670f4a1..28cbebcbd1 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.regressions.test.ts @@ -311,4 +311,57 @@ describe("tanstack-query/query-mutation-missing-invalidation — regressions", ( expect(diagnostics).toHaveLength(1); expect(diagnostics[0].message).toContain("can leave"); }); + + it("stays silent for a magic-link delivery mutation", () => { + const sendMagicLink = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + export function useSendMagicLink() { + return useMutation({ + mutationFn: (params: { email: string }) => api.sendMagicLink(params), + }); + }`, + ); + expect(sendMagicLink.diagnostics).toHaveLength(0); + }); + + it("still reports generic send, notify, and email mutations", () => { + const diagnostics = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + export function useSendMessage() { + return useMutation({ + mutationFn: (message) => api.sendMessage(message), + }); + } + + export function useNotifyUser() { + return useMutation({ + mutationFn: (userId) => api.notifyUser(userId), + }); + } + + export function useEmailInvite() { + return useMutation({ + mutationFn: (email) => api.emailInvite(email), + }); + }`, + ).diagnostics; + + expect(diagnostics).toHaveLength(3); + }); + + it("stays silent when the magic-link signal comes from the mutation function", () => { + const result = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + export function useAuthenticationEmail() { + return useMutation({ + mutationFn: (email) => api.sendMagicLink(email), + }); + }`, + ); + + expect(result.diagnostics).toHaveLength(0); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.ts index c0cde4cb1e..37c2cf88ed 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-query/query-mutation-missing-invalidation.ts @@ -50,6 +50,7 @@ const READ_ONLY_MUTATION_WORDS = new Set([ "oauth", "pairing", "sign", + "magiclink", ]); // `sign` followed by one of these is an auth ACTION (signIn / signUp / From 0fbef9b01162d301167be6ca6b5263714610f4e5 Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:29:24 +0300 Subject: [PATCH 6/8] fix(rule): narrow live cancellation guards (#1763) * fix(async-defer-await): recognize 'live' as a liveness guard name The async-defer-await rule was firing on post-await liveness guards that check a 'live' flag, commonly used in React effects to detect unmounting during async operations. The issue: 'live' was not in the CANCELLATION_NAME_FRAGMENTS list, so guards like 'if (!run.live) return' were not recognized as staleness checks. The fix adds 'live' to the fragments list so it matches patterns like: - run.live - isLive - liveness - stillLive Closes #1758 Co-authored-by: Skosh * chore: add changeset for async-defer-await fix Co-authored-by: Skosh * fix(rule): narrow live cancellation guards --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh Co-authored-by: Aiden Bai --- .changeset/curly-hounds-invite.md | 5 +++ .../async-defer-await--run-live-guard.ts | 15 ++++++++ .../async-defer-await.regressions.test.ts | 35 +++++++++++++++++++ .../rules/performance/async-defer-await.ts | 4 ++- .../regressions/js-performance-rules.test.ts | 34 ++++++++++++++++++ 5 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 .changeset/curly-hounds-invite.md create mode 100644 packages/fuzz/corpus/regressions/async-defer-await--run-live-guard.ts diff --git a/.changeset/curly-hounds-invite.md b/.changeset/curly-hounds-invite.md new file mode 100644 index 0000000000..d3015d31dc --- /dev/null +++ b/.changeset/curly-hounds-invite.md @@ -0,0 +1,5 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +Fix an `async-defer-await` false positive on exact `live` and `isLive` liveness guards without exempting unrelated names that only contain the same text. diff --git a/packages/fuzz/corpus/regressions/async-defer-await--run-live-guard.ts b/packages/fuzz/corpus/regressions/async-defer-await--run-live-guard.ts new file mode 100644 index 0000000000..9a23aab6cf --- /dev/null +++ b/packages/fuzz/corpus/regressions/async-defer-await--run-live-guard.ts @@ -0,0 +1,15 @@ +// rule: async-defer-await +// verdict: pass +// weakness: name-heuristic +// source: issue #1758 + +declare const refreshSession: () => Promise; +declare const setOk: (value: boolean) => void; + +const run = { live: true }; + +export const effect = async () => { + const refreshed = await refreshSession(); + if (!run.live) return; + setOk(refreshed); +}; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.regressions.test.ts index f03c9a076e..f5b5ab7563 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.regressions.test.ts @@ -498,4 +498,39 @@ describe("performance/async-defer-await — regressions", () => { expect(result.parseErrors).toEqual([]); expect(result.diagnostics).toHaveLength(1); }); + + it("stays silent on a run.live liveness guard in a React effect", () => { + const result = runRule( + asyncDeferAwait, + ` + declare const refreshSession: () => Promise; + declare const setOk: (value: boolean) => void; + const run = { live: true }; + export const effect = async () => { + const refreshed = await refreshSession(); + if (!run.live) return; + setOk(refreshed); + }; + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it("still reports guards with unrelated names that contain live", () => { + const result = runRule( + asyncDeferAwait, + ` + declare const loadDelivery: () => Promise; + declare const deliverNow: boolean; + export const deliver = async () => { + const payload = await loadDelivery(); + if (!deliverNow) return; + console.log(payload); + }; + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.ts index 7a1a29beab..e063f0877b 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/performance/async-defer-await.ts @@ -119,6 +119,8 @@ const CANCELLATION_GUARD_NAMES: ReadonlySet = new Set([ "isActive", "stale", "isStale", + "live", + "isLive", "ignore", "signal", "abortSignal", @@ -202,7 +204,7 @@ const isCancellationGuardTest = (test: EsTreeNode | null): boolean => { // `controller.signal.aborted`, `this._destroyed`, `batch.aborted`, // `seq !== getSeq.current`, `token !== runToken`. for (const name of collectAllTestNames(test)) { - if (isCancellationLikeName(name)) return true; + if (CANCELLATION_GUARD_NAMES.has(name) || isCancellationLikeName(name)) return true; } return testReadsRefCurrent(test); }; diff --git a/packages/react-doctor/tests/regressions/js-performance-rules.test.ts b/packages/react-doctor/tests/regressions/js-performance-rules.test.ts index 924a3b7d6a..943d427cf8 100644 --- a/packages/react-doctor/tests/regressions/js-performance-rules.test.ts +++ b/packages/react-doctor/tests/regressions/js-performance-rules.test.ts @@ -1504,3 +1504,37 @@ describe("issue #543: js-tosorted-immutable is gated off for React Native / Expo expect(hits).toHaveLength(1); }); }); + +describe("performance/async-defer-await", () => { + it("stays silent on a run.live liveness guard in a React effect (issue #1758)", async () => { + const projectDir = setupReactProject(tempRoot, "async-defer-run-live", { + files: { + "src/liveness-guard.tsx": ` + "use client"; + import { useEffect, useState } from "react"; + + declare function refreshSession(): Promise; + + export function LivenessGuard() { + const [ok, setOk] = useState(null); + useEffect(() => { + const run = { live: true }; + void (async () => { + const refreshed = await refreshSession(); + if (!run.live) return; + setOk(refreshed); + })(); + return () => { + run.live = false; + }; + }, []); + return {String(ok)}; + } + `, + }, + }); + + const hits = await collectRuleHits(projectDir, "async-defer-await"); + expect(hits).toHaveLength(0); + }); +}); From 6ac8b71985123ce43f7219965e188bdecf11f7b8 Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:29:42 +0300 Subject: [PATCH 7/8] fix(rule): scope GET helper safety to exact calls (#1761) * fix(nextjs-no-side-effect-in-get-handler): track safe Headers passed through helpers Fixes #1757 The rule now tracks when a locally-constructed safe object (like `new Headers()`) is passed as an argument to a same-file helper function. The corresponding parameter in that helper is treated as safe, preventing false positives when the helper mutates the response headers. Added `collectHelperParameterSafeBindings` utility that maps call arguments to helper parameters, extending the set of safe bindings when scanning helper bodies. Regression tests added for: - Headers passed to helper as first parameter - Headers passed as second parameter - Headers passed via destructured parameter - Module-level Map still correctly flagged (not locally scoped) Co-authored-by: Skosh * chore: add changeset for #1757 fix Co-authored-by: Skosh * fix(rule): scope GET helper safety by call --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh Co-authored-by: Aiden Bai --- .changeset/fix-1757-headers-helper.md | 9 ++ ...ct-in-get-handler--local-headers-helper.ts | 15 +++ .../nextjs-no-side-effect-in-get-handler.ts | 71 +++++++--- .../collect-helper-parameter-safe-bindings.ts | 31 +++++ .../nextjs-get-side-effects.test.ts | 123 ++++++++++++++++++ 5 files changed, 229 insertions(+), 20 deletions(-) create mode 100644 .changeset/fix-1757-headers-helper.md create mode 100644 packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--local-headers-helper.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-helper-parameter-safe-bindings.ts diff --git a/.changeset/fix-1757-headers-helper.md b/.changeset/fix-1757-headers-helper.md new file mode 100644 index 0000000000..02413836cf --- /dev/null +++ b/.changeset/fix-1757-headers-helper.md @@ -0,0 +1,9 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +Fix false positive in `nextjs-no-side-effect-in-get-handler` when locally-built `Headers` object is passed to a same-file helper that mutates it. + +The rule now transfers locally-created response object safety through the exact same-file helper call. Calls that pass external state to the same helper remain reportable. + +Fixes #1757 diff --git a/packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--local-headers-helper.ts b/packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--local-headers-helper.ts new file mode 100644 index 0000000000..2b20a72715 --- /dev/null +++ b/packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--local-headers-helper.ts @@ -0,0 +1,15 @@ +// verdict: pass +// rule: nextjs-no-side-effect-in-get-handler +// weakness: copy-tracking +// source: GitHub issue #1757 +// file-path: src/app/api/proxy/route.ts + +const applyCachePolicy = (responseHeaders: Headers) => { + responseHeaders.set("Cache-Control", "max-age=60"); +}; + +export const GET = () => { + const responseHeaders = new Headers(); + applyCachePolicy(responseHeaders); + return new Response(null, { headers: responseHeaders }); +}; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-side-effect-in-get-handler.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-side-effect-in-get-handler.ts index 4e8f921d7d..8f717b8f1f 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-side-effect-in-get-handler.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-side-effect-in-get-handler.ts @@ -4,6 +4,7 @@ import { ROUTE_HANDLER_FILE_PATTERN, } from "../../constants/nextjs.js"; import { GET_HANDLER_BINDING_RESOLUTION_DEPTH } from "../../constants/thresholds.js"; +import { collectHelperParameterSafeBindings } from "../../utils/collect-helper-parameter-safe-bindings.js"; import { collectLocallyScopedCookieBindings } from "../../utils/collect-locally-scoped-cookie-bindings.js"; import { collectLocallyScopedSafeBindings } from "../../utils/collect-locally-scoped-safe-bindings.js"; import { defineRule } from "../../utils/define-rule.js"; @@ -203,24 +204,31 @@ const resolveGetHandlerBodies = ( // `destroySession()` whose body does `cookies().delete(...)`). Collect those // helper bodies so they're scanned alongside the handler body; helpers called // from within helpers are NOT followed. -const collectCalledSameFileHelperBodies = ( +interface HelperBodyInfo { + body: EsTreeNode; + helperFunction: EsTreeNode; + callExpression: EsTreeNodeOfType<"CallExpression">; +} + +const collectCalledSameFileHelpers = ( handlerBody: EsTreeNode, resolveBinding: (identifierName: string) => EsTreeNode | null, -): EsTreeNode[] => { - const helperBodies: EsTreeNode[] = []; - const visitedHelperNames = new Set(); +): HelperBodyInfo[] => { + const helpers: HelperBodyInfo[] = []; walkAst(handlerBody, (child: EsTreeNode) => { if (!isNodeOfType(child, "CallExpression")) return; if (!isNodeOfType(child.callee, "Identifier")) return; const helperName = child.callee.name; - if (visitedHelperNames.has(helperName)) return; - visitedHelperNames.add(helperName); const helperBinding = resolveBinding(helperName); if (!isFunctionLike(helperBinding) || !helperBinding.body) return; if (helperBinding.body === handlerBody) return; - helperBodies.push(helperBinding.body); + helpers.push({ + body: helperBinding.body, + helperFunction: helperBinding, + callExpression: child, + }); }); - return helperBodies; + return helpers; }; export const nextjsNoSideEffectInGetHandler = defineRule({ @@ -257,22 +265,45 @@ export const nextjsNoSideEffectInGetHandler = defineRule({ const handlerBodies = resolveGetHandlerBodies(node, resolveBinding); for (const handlerBody of handlerBodies) { - const bodiesToScan = [ - handlerBody, - ...collectCalledSameFileHelperBodies(handlerBody, resolveBinding), - ]; - for (const scanBody of bodiesToScan) { - const sideEffect = findSideEffect(scanBody, { - locallyScopedSafeBindings: collectLocallyScopedSafeBindings(scanBody), - locallyScopedCookieBindings: collectLocallyScopedCookieBindings(scanBody), - }); - if (!sideEffect) continue; + const locallyScopedSafeBindings = collectLocallyScopedSafeBindings(handlerBody); + const locallyScopedCookieBindings = collectLocallyScopedCookieBindings(handlerBody); + + const sideEffectInHandler = findSideEffect(handlerBody, { + locallyScopedSafeBindings, + locallyScopedCookieBindings, + }); + if (sideEffectInHandler) { const message = mutatingSegment - ? `This GET handler on the "/${mutatingSegment}" route performs a side effect (${sideEffect}) and is prone to CSRF vulnerabilities, since prefetching or a forged request can trigger it.` - : `This GET handler's side effect (${sideEffect}) is prone to CSRF vulnerabilities, since prefetching or a forged request can trigger it.`; + ? `This GET handler on the "/${mutatingSegment}" route performs a side effect (${sideEffectInHandler}) and is prone to CSRF vulnerabilities, since prefetching or a forged request can trigger it.` + : `This GET handler's side effect (${sideEffectInHandler}) is prone to CSRF vulnerabilities, since prefetching or a forged request can trigger it.`; context.report({ node, message }); return; } + + const helpers = collectCalledSameFileHelpers(handlerBody, resolveBinding); + for (const { body: helperBody, helperFunction, callExpression } of helpers) { + const helperParameterSafeBindings = collectHelperParameterSafeBindings( + callExpression, + helperFunction, + locallyScopedSafeBindings, + ); + const effectiveSafeBindings = new Set([ + ...locallyScopedSafeBindings, + ...helperParameterSafeBindings, + ]); + + const sideEffectInHelper = findSideEffect(helperBody, { + locallyScopedSafeBindings: effectiveSafeBindings, + locallyScopedCookieBindings, + }); + if (sideEffectInHelper) { + const message = mutatingSegment + ? `This GET handler on the "/${mutatingSegment}" route performs a side effect (${sideEffectInHelper}) and is prone to CSRF vulnerabilities, since prefetching or a forged request can trigger it.` + : `This GET handler's side effect (${sideEffectInHelper}) is prone to CSRF vulnerabilities, since prefetching or a forged request can trigger it.`; + context.report({ node, message }); + return; + } + } } }, }; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-helper-parameter-safe-bindings.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-helper-parameter-safe-bindings.ts new file mode 100644 index 0000000000..3c1e1a85c0 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-helper-parameter-safe-bindings.ts @@ -0,0 +1,31 @@ +import type { EsTreeNode } from "./es-tree-node.js"; +import { isFunctionLike } from "./is-function-like.js"; +import { isNodeOfType } from "./is-node-of-type.js"; + +export const collectHelperParameterSafeBindings = ( + callExpression: EsTreeNode, + helperFunction: EsTreeNode, + locallyScopedSafeBindings: Set, +): Set => { + const parameterSafeBindings = new Set(); + + if (!isNodeOfType(callExpression, "CallExpression") || !isFunctionLike(helperFunction)) { + return parameterSafeBindings; + } + + const helperParameters = helperFunction.params ?? []; + if (helperParameters.length === 0) return parameterSafeBindings; + + for (let argumentIndex = 0; argumentIndex < callExpression.arguments.length; argumentIndex++) { + const argument = callExpression.arguments[argumentIndex]; + if (!isNodeOfType(argument, "Identifier")) continue; + if (!locallyScopedSafeBindings.has(argument.name)) continue; + + const correspondingParameter = helperParameters[argumentIndex]; + if (!isNodeOfType(correspondingParameter, "Identifier")) continue; + + parameterSafeBindings.add(correspondingParameter.name); + } + + return parameterSafeBindings; +}; diff --git a/packages/react-doctor/tests/regressions/nextjs-get-side-effects.test.ts b/packages/react-doctor/tests/regressions/nextjs-get-side-effects.test.ts index e51a00e58a..3b8c1ec902 100644 --- a/packages/react-doctor/tests/regressions/nextjs-get-side-effects.test.ts +++ b/packages/react-doctor/tests/regressions/nextjs-get-side-effects.test.ts @@ -246,6 +246,106 @@ export async function GET() { expect(filterRule(diagnostics)).toHaveLength(0); }); + it("locally-built Headers passed to a helper that calls `.set()` (issue #1757)", async () => { + const diagnostics = await writeRouteAndLint( + "issue-1757-headers-helper", + "src/app/api/proxy/route.ts", + `import type { NextRequest } from "next/server"; + +function applyCachePolicy(responseHeaders: Headers) { + responseHeaders.set("Cache-Control", "public, max-age=3, stale-while-revalidate=10"); +} + +async function handler(request: NextRequest) { + const upstream = await fetch(\`https://api.example.com\${request.nextUrl.pathname}\`, { + method: request.method, + }); + const headers = new Headers(upstream.headers); + if (request.method === "GET" && upstream.ok) { + applyCachePolicy(headers); + } + return new Response(upstream.body, { status: upstream.status, headers }); +} + +export const GET = handler; +export const POST = handler; +`, + ); + expect(filterRule(diagnostics)).toHaveLength(0); + }); + + it("locally-built Headers in second parameter position", async () => { + const diagnostics = await writeRouteAndLint( + "issue-1757-second-param", + "src/app/api/cache/route.ts", + `import { NextResponse } from "next/server"; + +function setCacheHeaders(status: number, headers: Headers) { + headers.set("Cache-Control", "max-age=60"); +} + +export async function GET() { + const headers = new Headers(); + setCacheHeaders(200, headers); + return new NextResponse(null, { headers }); +} +`, + ); + expect(filterRule(diagnostics)).toHaveLength(0); + }); + + it("does not transfer a safe argument to a different helper", async () => { + const diagnostics = await writeRouteAndLint( + "issue-1757-helper-identity", + "src/app/api/config/route.ts", + `import { NextResponse } from "next/server"; + +const cache = new Map(); + +function applyHeaders(headers: Headers) { + headers.set("Cache-Control", "max-age=60"); +} + +function updateCache(cacheMap: Map) { + cacheMap.set("requests", Date.now()); +} + +export async function GET() { + const headers = new Headers(); + applyHeaders(headers); + updateCache(cache); + return new NextResponse(null, { headers }); +} +`, + ); + const hits = filterRule(diagnostics); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("cacheMap.set()"); + }); + + it("reports when one helper receives both safe and external receivers", async () => { + const diagnostics = await writeRouteAndLint( + "issue-1757-mixed-helper-calls", + "src/app/api/config/route.ts", + `import { NextResponse } from "next/server"; + +const cache = new Map(); + +function applyValue(target: Headers | Map) { + target.set("Cache-Control", "max-age=60"); +} + +export async function GET() { + const headers = new Headers(); + applyValue(headers); + applyValue(cache); + return new NextResponse(null, { headers }); +} +`, + ); + expect(filterRule(diagnostics)).toHaveLength(1); + }); + it("aliased read-only `headers()` — `const h = headers(); h.get('user-agent')`", async () => { const diagnostics = await writeRouteAndLint( "issue-206-headers-alias-read", @@ -434,6 +534,29 @@ export async function GET() { expect(hits[0].message).toContain("cookies().delete()"); }); + it("module-level Map passed to helper still fires (not locally scoped)", async () => { + const diagnostics = await writeRouteAndLint( + "issue-1757-module-map-helper", + "src/app/api/cache/route.ts", + `import { NextResponse } from "next/server"; + +const cache = new Map(); + +function updateCache(cacheMap: Map, key: string) { + cacheMap.set(key, Date.now()); +} + +export async function GET(req: Request) { + updateCache(cache, req.url); + return NextResponse.json({ ok: true }); +} +`, + ); + const hits = filterRule(diagnostics); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("cacheMap.set()"); + }); + it("read-only GET on a mutating route segment `/logout` does NOT fire", async () => { // The route name is a hint, not proof — a GET that only returns JSON has // no side effect for a forged request or prefetch to trigger. From ff7dd679e8b9939a7dd8f828a530559a275836f7 Mon Sep 17 00:00:00 2001 From: Skosh Date: Sun, 6 Sep 2026 00:37:32 +0300 Subject: [PATCH 8/8] fix(rule): recognize owned effect cleanup paths (#1762) * test: add failing tests for issue #1756 false positives Covers three patterns: - Chained timer cleared by helper (timer reassigned, helper clears current value) - Timer allocated in nested function after await with guard - AbortController cleanup via abort event handler Co-authored-by: Skosh * docs: document issue #1756 root causes with failing regression tests Three false positive patterns in effect-needs-cleanup: 1. **Chained timer**: Timer reassigned inside its own callback, cleared by helper. Rule doesn't recognize mutable handle semantics - clearTimeout(timer) clears whatever is currently in the variable. 2. **Async guarded allocation**: Guard in async caller protects sync callee allocation. Rule recognizes guards inside promise callbacks (#1241) but not guards protecting function calls that contain allocations. 3. **AbortController delegation**: signal.addEventListener('abort', ...) removes listener, cleanup calls controller.abort(). Rule recognizes direct {signal} but not event-based delegation pattern. Tests currently fail as expected. Implementations need to extend: - Timer cleanup to understand mutable handles - Guard tracking to follow call boundaries - AbortController detection to recognize abort event delegation Related: #306, #1241, #1594, #1736 Co-authored-by: Skosh * wip: partial AbortController delegation fix (needs debugging) Added `hasAbortSignalDelegatedCleanup` helper to recognize the pattern: - signal.addEventListener('abort', () => removeEventListener(...)) - cleanup calls controller.abort() Logic handles both direct (controller.signal) and destructured (const { signal } = controller) signal references. Current status: Helper added and integrated into effectHasCleanupForUsage, but tests still fail. Needs debugging of AST traversal logic to correctly identify and match the abort listener with the cleanup abort() call. The other two patterns (chained timer, async guard) still need implementation. Co-authored-by: Skosh * fix(rule): recognize owned effect cleanup paths * fix(rule): preserve unsafe timer diagnostics --------- Co-authored-by: Cursor Agent Co-authored-by: Skosh Co-authored-by: Aiden Bai --- .changeset/calm-timers-listen.md | 5 + ...eeds-cleanup--issue-1756-abort-handler.tsx | 18 + ...eeds-cleanup--issue-1756-chained-timer.tsx | 27 ++ ...leanup--issue-1756-guarded-async-timer.tsx | 27 ++ .../effect-needs-cleanup-issue-1756.test.ts | 273 +++++++++++ .../state-and-effects/effect-needs-cleanup.ts | 425 +++++++++++++++++- 6 files changed, 764 insertions(+), 11 deletions(-) create mode 100644 .changeset/calm-timers-listen.md create mode 100644 packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-abort-handler.tsx create mode 100644 packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-chained-timer.tsx create mode 100644 packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-guarded-async-timer.tsx create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup-issue-1756.test.ts diff --git a/.changeset/calm-timers-listen.md b/.changeset/calm-timers-listen.md new file mode 100644 index 0000000000..fe086db916 --- /dev/null +++ b/.changeset/calm-timers-listen.md @@ -0,0 +1,5 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +Avoid `effect-needs-cleanup` diagnostics for owned chained timers, guarded post-await timers, and listeners released through an abort handler. diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-abort-handler.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-abort-handler.tsx new file mode 100644 index 0000000000..ff03df36d8 --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-abort-handler.tsx @@ -0,0 +1,18 @@ +// rule: effect-needs-cleanup +// weakness: cleanup-provenance +// source: issue #1756 +// verdict: pass +import { useEffect } from "react"; + +export const AbortHandlerCleanup = () => { + useEffect(() => { + const controller = new AbortController(); + const onChange = () => {}; + document.addEventListener("visibilitychange", onChange); + controller.signal.addEventListener("abort", () => { + document.removeEventListener("visibilitychange", onChange); + }); + return () => controller.abort(); + }, []); + return null; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-chained-timer.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-chained-timer.tsx new file mode 100644 index 0000000000..ebf3f9bf5a --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-chained-timer.tsx @@ -0,0 +1,27 @@ +// rule: effect-needs-cleanup +// weakness: cleanup-provenance +// source: issue #1756 +// verdict: pass +import { useEffect } from "react"; + +export const ChainedTimer = () => { + useEffect(() => { + let timer: ReturnType | null = null; + const clearTimer = () => { + if (timer !== null) { + clearTimeout(timer); + timer = null; + } + }; + const schedule = (callback: () => void) => { + timer = setTimeout(callback, 1000); + }; + const advance = () => { + timer = null; + schedule(advance); + }; + schedule(advance); + return () => clearTimer(); + }, []); + return null; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-guarded-async-timer.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-guarded-async-timer.tsx new file mode 100644 index 0000000000..1dc042348f --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1756-guarded-async-timer.tsx @@ -0,0 +1,27 @@ +// rule: effect-needs-cleanup +// weakness: async-lifecycle-provenance +// source: issue #1756 +// verdict: pass +import { useEffect } from "react"; + +declare const refresh: () => Promise; + +export const GuardedAsyncTimer = () => { + useEffect(() => { + const run = { live: true }; + let timer: ReturnType | null = null; + const schedule = () => { + timer = setTimeout(() => {}, 1000); + }; + void (async () => { + await refresh(); + if (!run.live) return; + schedule(); + })(); + return () => { + run.live = false; + if (timer !== null) clearTimeout(timer); + }; + }, []); + return null; +}; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup-issue-1756.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup-issue-1756.test.ts new file mode 100644 index 0000000000..1f0839f6ca --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup-issue-1756.test.ts @@ -0,0 +1,273 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { effectNeedsCleanup } from "./effect-needs-cleanup.js"; + +const runEffectNeedsCleanup = (code: string) => runRule(effectNeedsCleanup, code); + +describe("effect-needs-cleanup issue #1756", () => { + it("accepts chained timers cleared by the returned cleanup helper", () => { + const result = runEffectNeedsCleanup(` + import { useEffect, useState } from "react"; + + const STEPS = [0, 400, 800]; + + const ChainedTimer = () => { + const [frame, setFrame] = useState(0); + const [loop, setLoop] = useState(0); + useEffect(() => { + let cancelled = false; + let timer: ReturnType | null = null; + let index = 0; + const startedAt = Date.now(); + const clearTimer = () => { + if (timer !== null) { + clearTimeout(timer); + timer = null; + } + }; + const scheduleAt = (at: number, callback: () => void) => { + timer = setTimeout(callback, Math.max(0, at - (Date.now() - startedAt))); + }; + const advance = () => { + timer = null; + if (cancelled) return; + setFrame(index); + index += 1; + if (index < STEPS.length) { + scheduleAt(STEPS[index], advance); + } else { + scheduleAt(2000, () => { + if (!cancelled) setLoop((count) => count + 1); + }); + } + }; + const onVisibilityChange = () => { + if (cancelled) return; + if (document.hidden) clearTimer(); + else setLoop((count) => count + 1); + }; + document.addEventListener("visibilitychange", onVisibilityChange); + if (!document.hidden) scheduleAt(STEPS[0], advance); + return () => { + cancelled = true; + clearTimer(); + document.removeEventListener("visibilitychange", onVisibilityChange); + }; + }, [loop]); + return {frame}; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it("reports timer handle overwrites before the previous timer fires", () => { + const result = runEffectNeedsCleanup(` + import { useEffect } from "react"; + + const UnsafeTimers = () => { + useEffect(() => { + let timer: ReturnType | null = null; + const schedule = () => { + timer = setTimeout(() => {}, 1000); + }; + schedule(); + schedule(); + return () => { + if (timer !== null) clearTimeout(timer); + }; + }, []); + return null; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("accepts a guarded timer scheduled through a helper after an await", () => { + const result = runEffectNeedsCleanup(` + import { useEffect, useState } from "react"; + + declare function refresh(): Promise<"refreshed" | "invalid">; + + const AsyncBoundedTimer = () => { + const [unproven, setUnproven] = useState(false); + useEffect(() => { + const run = { live: true }; + let settleTimer: ReturnType | null = null; + const refreshAndBound = () => { + settleTimer = setTimeout(() => { + settleTimer = null; + if (run.live) setUnproven(true); + }, 4000); + }; + void (async () => { + const outcome = await refresh(); + if (!run.live) return; + if (outcome === "refreshed") { + refreshAndBound(); + return; + } + setUnproven(true); + })(); + return () => { + run.live = false; + if (settleTimer !== null) clearTimeout(settleTimer); + }; + }, []); + return {String(unproven)}; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it("reports a helper timer scheduled after an await without a lifecycle guard", () => { + const result = runEffectNeedsCleanup(` + import { useEffect } from "react"; + + declare function refresh(): Promise; + + const UnsafeAsyncTimer = () => { + useEffect(() => { + let timer: ReturnType | null = null; + const schedule = () => { + timer = setTimeout(() => {}, 1000); + }; + void (async () => { + await refresh(); + schedule(); + })(); + return () => { + if (timer !== null) clearTimeout(timer); + }; + }, []); + return null; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("reports an interruption between the lifecycle guard and timer helper", () => { + const result = runEffectNeedsCleanup(` + import { useEffect } from "react"; + + declare function refresh(): Promise; + + const UnsafeInterruptedTimer = () => { + useEffect(() => { + const run = { live: true }; + let timer: ReturnType | null = null; + const schedule = () => { + timer = setTimeout(() => {}, 1000); + }; + void (async () => { + await refresh(); + if (!run.live) return; + await refresh(); + schedule(); + })(); + return () => { + run.live = false; + if (timer !== null) clearTimeout(timer); + }; + }, []); + return null; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("reports guarded async timer work started repeatedly by the effect", () => { + const result = runEffectNeedsCleanup(` + import { useEffect } from "react"; + + declare function refresh(): Promise; + + const UnsafeRepeatedAsyncTimer = () => { + useEffect(() => { + const run = { live: true }; + let timer: ReturnType | null = null; + const schedule = () => { + timer = setTimeout(() => {}, 1000); + }; + for (let index = 0; index < 2; index += 1) { + void (async () => { + await refresh(); + if (!run.live) return; + schedule(); + })(); + } + return () => { + run.live = false; + if (timer !== null) clearTimeout(timer); + }; + }, []); + return null; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("accepts a listener released by an abort handler when cleanup aborts the controller", () => { + const result = runEffectNeedsCleanup(` + import { useEffect, useState } from "react"; + + const AbortTeardown = () => { + const [visible, setVisible] = useState(true); + useEffect(() => { + const controller = new AbortController(); + const { signal } = controller; + const onChange = () => setVisible(!document.hidden); + document.addEventListener("visibilitychange", onChange); + signal.addEventListener( + "abort", + () => { + document.removeEventListener("visibilitychange", onChange); + }, + { once: true }, + ); + return () => { + controller.abort(); + }; + }, []); + return {visible ? "visible" : "hidden"}; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it("reports an abort handler that removes a different listener", () => { + const result = runEffectNeedsCleanup(` + import { useEffect } from "react"; + + const UnsafeAbortTeardown = () => { + useEffect(() => { + const controller = new AbortController(); + const onChange = () => {}; + const otherHandler = () => {}; + document.addEventListener("visibilitychange", onChange); + controller.signal.addEventListener("abort", () => { + document.removeEventListener("visibilitychange", otherHandler); + }); + return () => controller.abort(); + }, []); + return null; + }; + `); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts index 6cbdbf6c0d..afb2184199 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts @@ -2641,6 +2641,123 @@ const getListenerAbortControllerKey = ( return null; }; +const resolveAbortSignalControllerKey = ( + signalExpression: EsTreeNode, + context: RuleContext, +): string | null => { + const unwrappedSignal = stripParenExpression(signalExpression); + if ( + isNodeOfType(unwrappedSignal, "MemberExpression") && + !unwrappedSignal.computed && + isNodeOfType(unwrappedSignal.property, "Identifier") && + unwrappedSignal.property.name === "signal" + ) { + return resolveExpressionKey(unwrappedSignal.object, context); + } + if (!isNodeOfType(unwrappedSignal, "Identifier")) return null; + const signalSymbol = context.scopes.symbolFor(unwrappedSignal); + if (!signalSymbol) return null; + const signalInitializer = signalSymbol.initializer + ? stripParenExpression(signalSymbol.initializer) + : null; + if ( + isNodeOfType(signalInitializer, "MemberExpression") && + !signalInitializer.computed && + isNodeOfType(signalInitializer.property, "Identifier") && + signalInitializer.property.name === "signal" + ) { + return resolveExpressionKey(signalInitializer.object, context); + } + const bindingProperty = signalSymbol.bindingIdentifier.parent; + if ( + !isNodeOfType(bindingProperty, "Property") || + getStaticPropertyKeyName(bindingProperty) !== "signal" || + !isNodeOfType(bindingProperty.parent, "ObjectPattern") || + !isNodeOfType(bindingProperty.parent.parent, "VariableDeclarator") || + !bindingProperty.parent.parent.init + ) { + return null; + } + return resolveExpressionKey(bindingProperty.parent.parent.init, context); +}; + +const getDelegatedListenerAbortControllerKey = ( + usage: SubscribeLikeUsage, + context: RuleContext, +): string | null => { + if ( + usage.registrationVerbName !== "addEventListener" || + !isNodeOfType(usage.node, "CallExpression") + ) { + return null; + } + const registrationCall = usage.node; + const registrationOwner = findEnclosingFunction(usage.node); + if (!registrationOwner || !isFunctionLike(registrationOwner)) return null; + let matchingControllerKey: string | null = null; + walkAst(registrationOwner.body, (child: EsTreeNode) => { + if (matchingControllerKey) return false; + if (child !== registrationOwner.body && isFunctionLike(child)) return false; + if (!isNodeOfType(child, "CallExpression") || getCalleeName(child) !== "addEventListener") { + return; + } + const callee = stripParenExpression(child.callee); + if (!isNodeOfType(callee, "MemberExpression")) return; + const eventName = child.arguments[0] ? stripParenExpression(child.arguments[0]) : null; + if (!isNodeOfType(eventName, "Literal") || eventName.value !== "abort") return; + const controllerKey = resolveAbortSignalControllerKey(callee.object, context); + if (!controllerKey) return; + const handler = resolveStableValue(child.arguments[1], context); + if (!handler || !isFunctionLike(handler) || handler.async || handler.generator) return; + const matchingRemovalCalls: EsTreeNode[] = []; + walkAst(handler.body, (handlerChild: EsTreeNode) => { + if (handlerChild !== handler.body && isFunctionLike(handlerChild)) return false; + if (!isNodeOfType(handlerChild, "CallExpression")) return; + const removalCallee = stripParenExpression(handlerChild.callee); + if ( + !isNodeOfType(removalCallee, "MemberExpression") || + getCalleeName(handlerChild) !== "removeEventListener" || + resolveResourceIdentityKey(removalCallee.object, context) !== usage.receiverKey || + resolveResourceIdentityKey(handlerChild.arguments[0], context) !== usage.eventKey || + resolveResourceIdentityKey(handlerChild.arguments[1], context) !== usage.handlerKey || + !doEventListenerCapturesMatch( + registrationCall.arguments[2], + handlerChild.arguments[2], + context, + true, + ) + ) { + return; + } + matchingRemovalCalls.push(handlerChild); + }); + if (doNodesCoverEveryPathFromFunctionEntry(handler, matchingRemovalCalls, context)) { + matchingControllerKey = controllerKey; + return false; + } + }); + return matchingControllerKey; +}; + +const getAbortSignalListenerControllerKey = ( + usage: SubscribeLikeUsage, + context: RuleContext, +): string | null => { + if ( + usage.registrationVerbName !== "addEventListener" || + !isNodeOfType(usage.node, "CallExpression") + ) { + return null; + } + const eventName = usage.node.arguments[0] ? stripParenExpression(usage.node.arguments[0]) : null; + const callee = stripParenExpression(usage.node.callee); + return isNodeOfType(eventName, "Literal") && + eventName.value === "abort" && + isNodeOfType(callee, "MemberExpression") + ? resolveAbortSignalControllerKey(callee.object, context) + : null; +}; + const findEnclosingForEachCall = (node: EsTreeNode): EsTreeNodeOfType<"CallExpression"> | null => { const callbackNode = isFunctionLike(node) ? node : findEnclosingFunction(node); if ( @@ -3990,8 +4107,9 @@ const collectDeferredUsageGuardStates = ( callback: EsTreeNode, usageNode: EsTreeNode, context: RuleContext, + allowAsyncCallback = false, ): BooleanGuardState[] => { - if (!isFunctionLike(callback) || callback.async) return []; + if (!isFunctionLike(callback) || (callback.async && !allowAsyncCallback)) return []; const guardStates: BooleanGuardState[] = []; walkAst(callback.body, (child: EsTreeNode) => { if (child !== callback.body && isFunctionLike(child)) return false; @@ -4129,6 +4247,70 @@ const isEffectLocalLifecycleGuard = ( }); }; +const isEffectLocalObjectLifecycleGuard = ( + callback: EsTreeNode, + guardState: BooleanGuardState, + cleanupFunctions: ReadonlyArray, + context: RuleContext, +): boolean => { + const guardMemberExpressions: EsTreeNodeOfType<"MemberExpression">[] = []; + walkAst(guardState.guardNode, (child: EsTreeNode) => { + if ( + guardMemberExpressions.length === 0 && + isNodeOfType(child, "MemberExpression") && + !child.computed && + isNodeOfType(child.object, "Identifier") && + isNodeOfType(child.property, "Identifier") && + resolveExpressionKey(child, context) === guardState.key + ) { + guardMemberExpressions.push(child); + return false; + } + }); + const guardMemberExpression = guardMemberExpressions[0]; + if (!guardMemberExpression) return false; + const objectSymbol = context.scopes.symbolFor(guardMemberExpression.object); + const initializer = objectSymbol?.initializer + ? stripParenExpression(objectSymbol.initializer) + : null; + if ( + !objectSymbol || + objectSymbol.kind !== "const" || + !isNodeOfType(objectSymbol.declarationNode, "VariableDeclarator") || + findEnclosingFunction(objectSymbol.declarationNode) !== callback || + !isNodeOfType(initializer, "ObjectExpression") || + initializer.properties.length !== 1 + ) { + return false; + } + const initialProperty = initializer.properties[0]; + if ( + !isNodeOfType(initialProperty, "Property") || + getStaticPropertyKeyName(initialProperty) !== getStaticPropertyKeyName(guardMemberExpression) || + readStaticBoolean(initialProperty.value) !== !guardState.value + ) { + return false; + } + return objectSymbol.references.every((reference) => { + const memberExpression = getOutermostMemberReference(reference.identifier); + if ( + !isNodeOfType(memberExpression, "MemberExpression") || + resolveExpressionKey(memberExpression, context) !== guardState.key + ) { + return false; + } + if (!isWithinAssignmentTarget(reference.identifier)) return true; + const assignment = memberExpression.parent; + return ( + isNodeOfType(assignment, "AssignmentExpression") && + assignment.operator === "=" && + assignment.left === memberExpression && + readStaticBoolean(assignment.right) === guardState.value && + cleanupFunctions.includes(findEnclosingFunction(assignment) ?? assignment) + ); + }); +}; + const hasPotentialInterruptionAfterGuard = ( callback: EsTreeNode, guardState: BooleanGuardState, @@ -4163,6 +4345,60 @@ const hasPotentialInterruptionAfterGuard = ( return hasPotentialInterruption; }; +const isDeferredHelperInvocationProtectedByEffectLifecycleGuard = ( + callback: EsTreeNode, + invocationOwner: EsTreeNode, + invocationCall: EsTreeNode, + cleanupReturns: ReadonlyArray, + context: RuleContext, +): boolean => { + const invocationOwnerRoot = findTransparentExpressionRoot(invocationOwner); + const directInvocation = + isNodeOfType(invocationOwnerRoot.parent, "CallExpression") && + invocationOwnerRoot.parent.callee === invocationOwnerRoot + ? invocationOwnerRoot.parent + : findSingleDirectInvocation(invocationOwner, callback, context); + if ( + !isFunctionLike(invocationOwner) || + !invocationOwner.async || + invocationOwner.generator || + !directInvocation || + findEnclosingFunction(directInvocation) !== callback || + !collectEffectInvokedFunctions(callback, context.scopes).has(invocationOwner) + ) { + return false; + } + let invocationAncestor = directInvocation.parent; + while (invocationAncestor && invocationAncestor !== callback) { + if ( + isNodeOfType(invocationAncestor, "ForStatement") || + isNodeOfType(invocationAncestor, "ForInStatement") || + isNodeOfType(invocationAncestor, "ForOfStatement") || + isNodeOfType(invocationAncestor, "WhileStatement") || + isNodeOfType(invocationAncestor, "DoWhileStatement") + ) { + return false; + } + invocationAncestor = invocationAncestor.parent; + } + const cleanupFunctions = cleanupReturns.flatMap((cleanupReturn) => { + if (!isNodeOfType(cleanupReturn, "ReturnStatement") || !cleanupReturn.argument) return []; + const cleanupFunction = resolveStableValue(cleanupReturn.argument, context); + return cleanupFunction && isFunctionLike(cleanupFunction) ? [cleanupFunction] : []; + }); + if (cleanupFunctions.length !== cleanupReturns.length) return false; + return collectDeferredUsageGuardStates(invocationOwner, invocationCall, context, true).some( + (guardState) => + (isEffectLocalLifecycleGuard(callback, guardState, cleanupFunctions, context) || + isEffectLocalObjectLifecycleGuard(callback, guardState, cleanupFunctions, context)) && + !hasPotentialInterruptionAfterGuard(invocationOwner, guardState, invocationCall, context) && + !deferredUsageWritesGuardBeforeUsage(invocationOwner, invocationCall, guardState, context) && + cleanupReturns.every((cleanupReturn) => + cleanupReturnInvalidatesGuard(cleanupReturn, guardState, context), + ), + ); +}; + const getNumericReactRefCurrentKey = ( expression: EsTreeNode, context: RuleContext, @@ -4781,10 +5017,128 @@ const getOutermostMemberReference = (identifier: EsTreeNode): EsTreeNode => { return findTransparentExpressionRoot(expression); }; +const getTimerCallbackIdentityKeys = ( + usage: SubscribeLikeUsage, + context: RuleContext, +): ReadonlySet => { + const callbackIdentityKeys = new Set(); + const invocationCallbackIdentityKeys = new Set(); + const callbackKey = getUsageCallbackKey(usage, context); + if (callbackKey) callbackIdentityKeys.add(callbackKey); + if (usage.kind !== "timer" || !isNodeOfType(usage.node, "CallExpression")) { + return callbackIdentityKeys; + } + const callbackArgument = usage.node.arguments[0]; + if (!callbackArgument || !isNodeOfType(callbackArgument, "Identifier")) { + return callbackIdentityKeys; + } + const callbackSymbol = context.scopes.symbolFor(callbackArgument); + const usageFunction = callbackSymbol + ? findEnclosingFunction(callbackSymbol.bindingIdentifier) + : null; + if (!callbackSymbol || !usageFunction || !isFunctionLike(usageFunction)) { + return callbackIdentityKeys; + } + const callbackParameterIndex = usageFunction.params.findIndex( + (parameter) => parameter === callbackSymbol.bindingIdentifier, + ); + const functionBindingIdentifier = getFunctionBindingIdentifier(usageFunction); + const functionSymbol = functionBindingIdentifier + ? context.scopes.symbolFor(functionBindingIdentifier) + : null; + if (callbackParameterIndex < 0 || !functionSymbol) return callbackIdentityKeys; + for (const reference of functionSymbol.references) { + const invocationCall = findDirectCallForReference(reference.identifier); + if (!isNodeOfType(invocationCall, "CallExpression")) { + return callbackIdentityKeys; + } + const invocationArgument = invocationCall.arguments[callbackParameterIndex]; + if (!invocationArgument || !isAstNode(invocationArgument)) { + return callbackIdentityKeys; + } + const invocationCallbackKey = resolveExpressionKey(invocationArgument, context); + if (invocationCallbackKey) invocationCallbackIdentityKeys.add(invocationCallbackKey); + } + for (const identityKey of invocationCallbackIdentityKeys) { + callbackIdentityKeys.add(identityKey); + } + return callbackIdentityKeys; +}; + +const isTimerCallbackForSameHandle = ( + functionNode: EsTreeNode, + usage: SubscribeLikeUsage, + allUsages: ReadonlyArray, + context: RuleContext, +): boolean => { + if (!isFunctionLike(functionNode) || usage.handleKey === null) return false; + const functionIdentityKeys = getFunctionIdentityKeys(functionNode, context); + return allUsages.some((candidateUsage) => { + if ( + candidateUsage.kind !== "timer" || + candidateUsage.handleKey !== usage.handleKey || + findEnclosingFunction(candidateUsage.node) === functionNode + ) { + return false; + } + const candidateCallbackIdentityKeys = getTimerCallbackIdentityKeys(candidateUsage, context); + return [...functionIdentityKeys].some((identityKey) => + candidateCallbackIdentityKeys.has(identityKey), + ); + }); +}; + +const hasOnlyOwnedTimerHelperInvocations = ( + callback: EsTreeNode, + usage: SubscribeLikeUsage, + usageFunction: EsTreeNode, + functionSymbol: SymbolDescriptor, + context: RuleContext, +): boolean => { + const invocationsByOwner = new Map(); + let directEffectInvocationCount = 0; + for (const reference of functionSymbol.references) { + const invocationCall = findDirectCallForReference(reference.identifier); + const invocationOwner = invocationCall ? findEnclosingFunction(invocationCall) : null; + if (!invocationCall || !invocationOwner || !isFunctionLike(invocationOwner)) return false; + if (invocationOwner === callback) { + directEffectInvocationCount += 1; + } else if (!isTimerCallbackForSameHandle(invocationOwner, usage, [usage], context)) { + return false; + } + const ownerInvocations = invocationsByOwner.get(invocationOwner) ?? []; + ownerInvocations.push(invocationCall); + invocationsByOwner.set(invocationOwner, ownerInvocations); + } + if (directEffectInvocationCount > 1) return false; + return [...invocationsByOwner.entries()].every(([invocationOwner, invocations]) => + invocations.every((invocation, invocationIndex) => + invocations + .slice(invocationIndex + 1) + .every( + (laterInvocation) => + !canNodeReachLaterNodeWithinFunction( + invocation, + laterInvocation, + invocationOwner, + context, + ) && + !canNodeReachLaterNodeWithinFunction( + laterInvocation, + invocation, + invocationOwner, + context, + ), + ), + ), + ); +}; + const hasOnlySafeHandleStorageAssignments = ( usage: SubscribeLikeUsage, handleStorageSymbol: SymbolDescriptor, usageAssignment: EsTreeNodeOfType<"AssignmentExpression">, + allUsages: ReadonlyArray, context: RuleContext, ): boolean => handleStorageSymbol.references.every((reference) => { @@ -4803,13 +5157,32 @@ const hasOnlySafeHandleStorageAssignments = ( return false; } const assignedValue = stripParenExpression(assignment.right); + const assignmentOwner = findEnclosingFunction(assignment); + const assignedTimerUsage = allUsages.find( + (candidateUsage) => + candidateUsage.kind === "timer" && + candidateUsage.node === assignedValue && + candidateUsage.handleKey === usage.handleKey, + ); + const currentUsageOwner = findEnclosingFunction(usage.node); + const currentUsageOwnerKeys = currentUsageOwner + ? getFunctionIdentityKeys(currentUsageOwner, context) + : new Set(); + if ( + assignedTimerUsage && + ((assignmentOwner && + isTimerCallbackForSameHandle(assignmentOwner, usage, allUsages, context)) || + currentUsageOwnerKeys.has(getUsageCallbackKey(assignedTimerUsage, context) ?? "")) + ) { + return true; + } const isNullishReset = (isNodeOfType(assignedValue, "Literal") && assignedValue.value === null) || (isNodeOfType(assignedValue, "Identifier") && assignedValue.name === "undefined" && context.scopes.isGlobalReference(assignedValue)); - const assignmentOwner = findEnclosingFunction(assignment); if (!isNullishReset || !assignmentOwner || !isFunctionLike(assignmentOwner)) return false; + if (isTimerCallbackForSameHandle(assignmentOwner, usage, allUsages, context)) return true; const matchingReleaseCalls: EsTreeNode[] = []; walkAst(assignmentOwner.body, (child: EsTreeNode) => { if (child !== assignmentOwner.body && isFunctionLike(child)) return false; @@ -4888,10 +5261,28 @@ const hasEffectOwnedNestedTimerCleanup = ( if ( handleStorageSymbol && isNodeOfType(usageAssignment, "AssignmentExpression") && - !hasOnlySafeHandleStorageAssignments(usage, handleStorageSymbol, usageAssignment, context) + !hasOnlySafeHandleStorageAssignments( + usage, + handleStorageSymbol, + usageAssignment, + allUsages, + context, + ) ) { return false; } + if (isTimerCallbackForSameHandle(usageFunction, usage, allUsages, context)) { + return true; + } + const functionBindingIdentifier = getFunctionBindingIdentifier(usageFunction); + const functionSymbol = functionBindingIdentifier + ? context.scopes.symbolFor(functionBindingIdentifier) + : null; + if (!functionSymbol || functionSymbol.references.length === 0) return false; + const singleInvocationCall = + functionSymbol.references.length === 1 + ? findDirectCallForReference(functionSymbol.references[0].identifier) + : null; const isSelfRescheduling = isSelfReschedulingOneShotTimer( usage, usageFunction, @@ -4901,7 +5292,9 @@ const hasEffectOwnedNestedTimerCleanup = ( if ( handleStorageSymbol && !isSelfRescheduling && - !hasLiveHandleOverwriteProtection(usageFunction, usage, context) + !hasLiveHandleOverwriteProtection(usageFunction, usage, context) && + !singleInvocationCall && + !hasOnlyOwnedTimerHelperInvocations(callback, usage, usageFunction, functionSymbol, context) ) { return false; } @@ -4919,11 +5312,6 @@ const hasEffectOwnedNestedTimerCleanup = ( } usageAncestor = usageAncestor.parent; } - const functionBindingIdentifier = getFunctionBindingIdentifier(usageFunction); - const functionSymbol = functionBindingIdentifier - ? context.scopes.symbolFor(functionBindingIdentifier) - : null; - if (!functionSymbol || functionSymbol.references.length === 0) return false; const selfSchedulingReferences = functionSymbol.references.filter( (reference) => isSelfRescheduling && @@ -4969,7 +5357,19 @@ const hasEffectOwnedNestedTimerCleanup = ( if (!invocationOwner || !isFunctionLike(invocationOwner) || invocationOwner === usageFunction) { return false; } - if (invocationOwner.async || invocationOwner.generator) return false; + if (invocationOwner.generator) return false; + if (invocationOwner.async) { + return isDeferredHelperInvocationProtectedByEffectLifecycleGuard( + callback, + invocationOwner, + invocationCall, + cleanupReturns, + context, + ); + } + if (isTimerCallbackForSameHandle(invocationOwner, usage, allUsages, context)) { + return true; + } if (invocationOwner === callback) { return doMatchingNodesCoverEveryPathAfterUsage( resolveCleanupPathAnchor(invocationCall, callback, context), @@ -7316,7 +7716,10 @@ const doesReleaseCallMatchUsage = ( } if ( releaseVerbName === "abort" && - releaseReceiverKey === getListenerAbortControllerKey(usage, context) + releaseReceiverKey === + (getListenerAbortControllerKey(usage, context) ?? + getDelegatedListenerAbortControllerKey(usage, context) ?? + getAbortSignalListenerControllerKey(usage, context)) ) { return true; }