From 499a0208fca5c0422b713bdedf2b83fcc8e29d20 Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Fri, 18 Sep 2026 02:29:04 +0800 Subject: [PATCH] fix: quiet four rule false positives from recent issues (#1814) --- .../quiet-wrapper-store-and-static-padding.md | 13 +++ ...get-handler--helper-constructed-headers.ts | 33 +++++++ ...te-updater--async-operation-run-helper.tsx | 33 +++++++ ...ing-invalidation--zustand-store-resync.tsx | 31 +++++++ ...g-invalidation--zustand-ui-state-write.tsx | 18 ++++ ...dynamic-padding--static-branch-ternary.tsx | 28 ++++++ .../src/plugin/constants/thresholds.ts | 4 + .../src/plugin/core-rule-registry-data.json | 2 +- ...-effect-in-get-handler.regressions.test.ts | 52 +++++++++++ .../nextjs-no-side-effect-in-get-handler.ts | 4 + ...llview-dynamic-padding.regressions.test.ts | 55 ++++++++++++ .../rn-scrollview-dynamic-padding.ts | 12 ++- .../no-impure-state-updater.test.ts | 62 +++++++++++++ .../no-impure-state-updater.ts | 50 ++++++++++- ...n-missing-invalidation.regressions.test.ts | 86 +++++++++++++++++++ .../query-mutation-missing-invalidation.ts | 70 +++++++++++++++ 16 files changed, 549 insertions(+), 4 deletions(-) create mode 100644 .changeset/quiet-wrapper-store-and-static-padding.md create mode 100644 packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--helper-constructed-headers.ts create mode 100644 packages/fuzz/corpus/regressions/no-impure-state-updater--async-operation-run-helper.tsx create mode 100644 packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--zustand-store-resync.tsx create mode 100644 packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--zustand-ui-state-write.tsx create mode 100644 packages/fuzz/corpus/regressions/rn-scrollview-dynamic-padding--static-branch-ternary.tsx diff --git a/.changeset/quiet-wrapper-store-and-static-padding.md b/.changeset/quiet-wrapper-store-and-static-padding.md new file mode 100644 index 0000000000..06e514c510 --- /dev/null +++ b/.changeset/quiet-wrapper-store-and-static-padding.md @@ -0,0 +1,13 @@ +--- +"react-doctor": patch +"oxlint-plugin-react-doctor": patch +"eslint-plugin-react-doctor": patch +--- + +Stop `no-impure-state-updater` reporting callbacks handed to a helper that merely runs them (`run(async () => setValue("x"))`). Only a wrapper that forwards its parameter into a React setter's updater slot still counts as an updater. + +Stop `nextjs-no-side-effect-in-get-handler` reporting `.set()` on a `Headers` object the helper constructs itself; mutations on stores the helper did not create still report. + +Treat a ternary between static values (`hasHeader ? 0 : 16`) as static spacing in `rn-scrollview-dynamic-padding`, and reword its recommendation to name the matching `contentInset` edge and its iOS-only scope. + +Accept a Zustand `store.setState(awaitedValue)` re-sync as a cache update in `query-mutation-missing-invalidation`; plain UI-state writes and non-store bindings still report. diff --git a/packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--helper-constructed-headers.ts b/packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--helper-constructed-headers.ts new file mode 100644 index 0000000000..5c549b4878 --- /dev/null +++ b/packages/fuzz/corpus/regressions/nextjs-no-side-effect-in-get-handler--helper-constructed-headers.ts @@ -0,0 +1,33 @@ +// verdict: pass +// rule: nextjs-no-side-effect-in-get-handler +// weakness: receiver-provenance +// source: GitHub issue #1808 +// file-path: app/api/download/route.ts + +declare function authorize(): Promise; + +function downloadHeaders({ + filename, + contentType, + contentLength, +}: { + filename: string; + contentType: string; + contentLength?: string | null; +}): Headers { + const headers = new Headers({ + "Content-Type": contentType, + "Content-Disposition": `attachment; filename="${filename}"`, + }); + if (contentLength) headers.set("Content-Length", contentLength); + return headers; +} + +export async function GET() { + const denied = await authorize(); + if (denied) return denied; + const upstream = await fetch("https://cdn.example.com/video.mp4"); + return new Response(upstream.body, { + headers: downloadHeaders({ filename: "x", contentType: "video/mp4", contentLength: "123" }), + }); +} diff --git a/packages/fuzz/corpus/regressions/no-impure-state-updater--async-operation-run-helper.tsx b/packages/fuzz/corpus/regressions/no-impure-state-updater--async-operation-run-helper.tsx new file mode 100644 index 0000000000..13f3c1ae45 --- /dev/null +++ b/packages/fuzz/corpus/regressions/no-impure-state-updater--async-operation-run-helper.tsx @@ -0,0 +1,33 @@ +// verdict: pass +// rule: no-impure-state-updater +// weakness: wrapper-transparency +// source: GitHub issue #1812 +import { useCallback, useState } from "react"; + +export function Repro() { + const [busy, setBusy] = useState(false); + const [value, setValue] = useState(""); + const run = useCallback(async (operation: () => Promise) => { + setBusy(true); + try { + await operation(); + } finally { + setBusy(false); + } + }, []); + const first = () => + run(async () => { + await Promise.resolve(); + setValue("first"); + }); + const second = () => + run(async () => { + await Promise.resolve(); + setValue("second"); + }); + return ( + + ); +} diff --git a/packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--zustand-store-resync.tsx b/packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--zustand-store-resync.tsx new file mode 100644 index 0000000000..e6b0b1521f --- /dev/null +++ b/packages/fuzz/corpus/regressions/query-mutation-missing-invalidation--zustand-store-resync.tsx @@ -0,0 +1,31 @@ +// verdict: pass +// rule: query-mutation-missing-invalidation +// weakness: library-idiom +// source: GitHub issue #1786 +import { useMutation } from "@tanstack/react-query"; +import { Button, Text, View } from "react-native"; +import { create } from "zustand"; + +declare function purchase(): Promise<"purchased" | "cancelled">; +declare function getMembership(): Promise<{ tier: string }>; +const useMembership = create<{ tier: string }>(() => ({ tier: "free" })); + +async function reconcileMembership() { + const membership = await getMembership(); + useMembership.setState(membership); +} + +export function Upgrade() { + const tier = useMembership((state) => state.tier); + const upgrade = useMutation({ + mutationFn: async () => { + if ((await purchase()) === "purchased") await reconcileMembership(); + }, + }); + return ( + + {tier} + ; + };`, + ], + [ + "a callback whose wrapper only calls it and stores the result", + `import { useState } from "react"; + const Panel = () => { + const [result, setResult] = useState(null); + const measure = (compute) => setResult(compute()); + const refresh = () => measure(() => { + localStorage.setItem("measured", "true"); + return 1; + }); + return ; + };`, + ], [ "a callback with adjacent side effect outside updater", `import { useState } from "react"; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-impure-state-updater.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-impure-state-updater.ts index 94ebea61b3..5edfa0bd02 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-impure-state-updater.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-impure-state-updater.ts @@ -1,3 +1,5 @@ +import type { Reference } from "eslint-scope"; +import { UPDATER_WRAPPER_RESOLUTION_DEPTH } from "../../constants/thresholds.js"; import { isDescendantScope } from "../../semantic/scope-analysis.js"; import type { ScopeAnalysis } from "../../semantic/scope-analysis.js"; import { defineRule } from "../../utils/define-rule.js"; @@ -16,7 +18,12 @@ import { stripParenExpression } from "../../utils/strip-paren-expression.js"; import { walkAst } from "../../utils/walk-ast.js"; import { getRef, resolveToFunction } from "./utils/effect/ast.js"; import { getProgramAnalysis } from "./utils/effect/get-program-analysis.js"; -import { getUseStateDecl, isStateSetterCall } from "./utils/effect/react.js"; +import type { ProgramAnalysis } from "./utils/effect/get-program-analysis.js"; +import { + getUseStateDecl, + isStateSetterCall, + resolveStateSetterReference, +} from "./utils/effect/react.js"; interface MemberCall { methodName: string; @@ -250,6 +257,40 @@ const isDefinitelySynchronousCallback = (callback: EsTreeNode, scopes: ScopeAnal ); }; +// `run(async () => setValue("x"))` eventually calls `setBusy`, but `run` +// invokes the callback itself — React never replays it as an updater. A +// wrapper only counts when it hands its first parameter straight into a +// setter's updater slot (`const update = (updater) => setCount(updater)`). +const isUpdaterConsumingSetterCall = ( + analysis: ProgramAnalysis, + scopes: ScopeAnalysis, + calleeReference: Reference, + remainingDepth = UPDATER_WRAPPER_RESOLUTION_DEPTH, +): boolean => { + if (resolveStateSetterReference(analysis, calleeReference)) return true; + if (remainingDepth <= 0) return false; + const updaterParameter = resolveToFunction(calleeReference)?.params[0]; + if (!updaterParameter || !isNodeOfType(updaterParameter, "Identifier")) return false; + const parameterSymbol = scopes.symbolFor(updaterParameter); + return Boolean( + parameterSymbol?.references.some((reference) => { + const forwardingCall = reference.identifier.parent; + if ( + !isNodeOfType(forwardingCall, "CallExpression") || + forwardingCall.arguments[0] !== reference.identifier || + !isNodeOfType(forwardingCall.callee, "Identifier") + ) { + return false; + } + const forwardedCalleeReference = getRef(analysis, forwardingCall.callee); + return ( + forwardedCalleeReference !== null && + isUpdaterConsumingSetterCall(analysis, scopes, forwardedCalleeReference, remainingDepth - 1) + ); + }), + ); +}; + const findImpureUpdaterOperation = (updater: EsTreeNode, scopes: ScopeAnalysis): string | null => { const analysis = getProgramAnalysis(updater); let operation: string | null = null; @@ -324,7 +365,12 @@ export const noImpureStateUpdater = defineRule({ const analysis = getProgramAnalysis(node); if (!analysis) return; const calleeReference = getRef(analysis, node.callee); - if (!calleeReference || !isStateSetterCall(analysis, calleeReference)) return; + if ( + !calleeReference || + !isUpdaterConsumingSetterCall(analysis, context.scopes, calleeReference) + ) { + return; + } const stateDeclarator = getUseStateDecl(analysis, calleeReference); if ( !isNodeOfType(stateDeclarator, "VariableDeclarator") || 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 28cbebcbd1..474d36ec86 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 @@ -364,4 +364,90 @@ describe("tanstack-query/query-mutation-missing-invalidation — regressions", ( expect(result.diagnostics).toHaveLength(0); }); + + // Issue #1786: the mutation re-fetches the membership and writes it into + // the Zustand store the UI reads from — the data owner is re-synced. + it("stays silent when a helper writes an awaited fetch into a same-file Zustand store", () => { + const result = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + import { create } from "zustand"; + declare function purchase(): Promise<"purchased" | "cancelled">; + declare function getMembership(): Promise<{ tier: string }>; + const useMembership = create<{ tier: string }>(() => ({ tier: "free" })); + async function reconcileMembership() { + const membership = await getMembership(); + useMembership.setState(membership); + } + export function Upgrade() { + const tier = useMembership((state) => state.tier); + const upgrade = useMutation({ + mutationFn: async () => { + if ((await purchase()) === "purchased") await reconcileMembership(); + }, + }); + return ; + }`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it("stays silent when onSuccess writes a destructured awaited response into an imported store", () => { + const result = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + import { useProfileStore } from "@/stores/profile"; + export function useRenameProfile() { + return useMutation({ + mutationFn: (name: string) => api.renameProfile(name), + onSuccess: async () => { + const { data } = await api.getProfile(); + useProfileStore.setState((state) => ({ ...state, profile: data })); + }, + }); + }`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it("still flags a Zustand write of UI state that never re-fetches", () => { + const result = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + import { create } from "zustand"; + const useUiStore = create(() => ({ isDialogOpen: false })); + export function useArchiveProject() { + return useMutation({ + mutationFn: (id: string) => api.archiveProject(id), + onSuccess: () => { + useUiStore.setState({ isDialogOpen: false }); + }, + }); + }`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + + it("still flags a setState on a same-file non-store binding fed by an awaited value", () => { + const result = runRule( + queryMutationMissingInvalidation, + `import { useMutation } from "@tanstack/react-query"; + import { useForm } from "./use-form"; + export function useSaveDraft() { + const form = useForm(); + return useMutation({ + mutationFn: (draft) => api.saveDraft(draft), + onSuccess: async () => { + const saved = await api.getDraft(); + form.setState(saved); + }, + }); + }`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); }); 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 37c2cf88ed..8e97c70981 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 @@ -10,6 +10,9 @@ import { defineRule } from "../../utils/define-rule.js"; import { enclosingComponentOrHookName } from "../../utils/enclosing-component-or-hook-name.js"; import { flattenCalleeName } from "../../utils/flatten-callee-name.js"; import { getCalleeName } from "../../utils/get-callee-name.js"; +import { resolveConstIdentifierAlias } from "../../utils/resolve-const-identifier-alias.js"; +import { resolveZustandStoreFactoryCall } from "../../utils/resolve-zustand-api.js"; +import { stripParenExpression } from "../../utils/strip-paren-expression.js"; import { tokenizeIdentifierWords } from "../../utils/tokenize-identifier-words.js"; import { walkAst } from "../../utils/walk-ast.js"; import type { EsTreeNode } from "../../utils/es-tree-node.js"; @@ -31,6 +34,7 @@ const MUTATION_LIFECYCLE_CALLBACK_NAMES = new Set([ "onMutate", ]); const FULL_PAGE_NAVIGATION_METHODS = new Set(["assign", "reload", "replace"]); +const EXTERNAL_STORE_WRITE_METHOD = "setState"; const MAX_HELPER_RESOLUTION_DEPTH = 3; // Words that mark a mutation as read-style: it fetches, checks, or produces @@ -201,6 +205,71 @@ const isFullPageNavigation = (node: EsTreeNode): boolean => { return false; }; +// True when `node` is, or reads a binding initialized by, an awaited +// expression — the shape of a value that just came back from the server. +const isAwaitedValueSource = (node: EsTreeNode, scopes: ScopeAnalysis): boolean => { + let didFindAwaitedValue = false; + walkAst(node, (child: EsTreeNode) => { + if (didFindAwaitedValue) return false; + if (isNodeOfType(child, "AwaitExpression")) { + didFindAwaitedValue = true; + return false; + } + if (!isNodeOfType(child, "Identifier")) return; + const symbol = resolveConstIdentifierAlias(child, scopes, true); + if ( + symbol?.initializer && + isNodeOfType(stripParenExpression(symbol.initializer), "AwaitExpression") + ) { + didFindAwaitedValue = true; + return false; + } + }); + return didFindAwaitedValue; +}; + +// A same-file Zustand store (`const useMembership = create(...)`), or an +// imported binding whose store we cannot see into and trust like the +// rule's other unresolvable callables. +const isExternalStoreReceiver = (receiver: EsTreeNode, scopes: ScopeAnalysis): boolean => { + if (!isNodeOfType(receiver, "Identifier")) return false; + const symbol = resolveConstIdentifierAlias(receiver, scopes); + if (!symbol) return false; + if (symbol.kind === "import") return true; + if (!symbol.initializer) return false; + const initializer = stripParenExpression(symbol.initializer); + return ( + isNodeOfType(initializer, "CallExpression") && + resolveZustandStoreFactoryCall(initializer, scopes) !== null + ); +}; + +// `useMembership.setState(membership)` after `const membership = await +// getMembership()` re-syncs the store that owns that server data, so the +// UI reading it is not stale. Only an awaited value qualifies: a plain +// UI-state write (`useUiStore.setState({ open: false })`) proves nothing +// about cached server data and still reports. +const isExternalStoreResyncFromServer = ( + callExpression: EsTreeNodeOfType<"CallExpression">, + scopes: ScopeAnalysis, +): boolean => { + const callee = callExpression.callee; + if ( + !isNodeOfType(callee, "MemberExpression") || + callee.computed || + !isNodeOfType(callee.property, "Identifier") || + callee.property.name !== EXTERNAL_STORE_WRITE_METHOD + ) { + return false; + } + const nextStateArgument = callExpression.arguments[0]; + return Boolean( + nextStateArgument && + isExternalStoreReceiver(stripParenExpression(callee.object), scopes) && + isAwaitedValueSource(nextStateArgument, scopes), + ); +}; + const mutationResultBindingName = ( mutationCall: EsTreeNodeOfType<"CallExpression">, ): string | null => { @@ -311,6 +380,7 @@ const createCacheUpdateDetector = (scopes: ScopeAnalysis): CacheUpdateDetector = if (isNodeOfType(node, "CallExpression")) { if (doesCallableSyncCache(node.callee, remainingDepth)) return true; + if (isExternalStoreResyncFromServer(node, scopes)) return true; // Handing the query client to a helper (`fetchDetails(queryClient)`) // delegates the cache update to it. return (node.arguments ?? []).some((argument) => isQueryClientValue(argument, scopes));