feat(rules): add no-stale-closure rule for detecting stale value bugs#476
feat(rules): add no-stale-closure rule for detecting stale value bugs#476devin-ai-integration[bot] wants to merge 2 commits into
no-stale-closure rule for detecting stale value bugs#476Conversation
Inspired by React compiler's HIR reactive identifier tracking, this rule
detects callbacks that are stabilized (via useCallback with empty deps or
useRef without updates) but close over reactive values (props, useState,
useContext, useReducer state) that will go stale after the first render.
Recommends useEffectEvent (React 19+) or useNonReactiveCallback as fixes.
Detection targets:
- useCallback(() => { ...reads reactive... }, [])
- useRef(() => { ...reads reactive... }) where ref.current is never reassigned
Correctly handles false positives:
- setState dispatchers (stable identity)
- useRef bindings (stable identity)
- useEffectEvent returns (stable identity)
- Callback parameters that shadow reactive names
- Local variables inside callback that shadow reactive names
- Setter-pattern names (set*)
- Property access positions (obj.prop)
- useCallback with non-empty deps
- useRef with reassigned .current
Includes 32 tests covering fail cases, pass cases, and real-world patterns
from open-source codebases.
Co-Authored-By: nisarg@million.dev <awesomenisarg@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: nisarg@million.dev <awesomenisarg@gmail.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fef46f8. Configure here.
| const getSimpleCalleeName = (node: EsTreeNode): string | null => { | ||
| if (!isNodeOfType(node, "CallExpression")) return null; | ||
| if (isNodeOfType(node.callee, "Identifier")) return node.callee.name; | ||
| return null; |
There was a problem hiding this comment.
Misses React namespace hooks
Medium Severity
getSimpleCalleeName only recognizes hook calls whose callee is a bare identifier, so React.useCallback, React.useState, and similar forms are ignored. Stale-closure analysis and reactive/stable binding collection never run for common namespace import styles.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit fef46f8. Configure here.
| if (stableNames.has(identifierName)) return; | ||
| if (SETTER_PATTERN.test(identifierName)) return; | ||
| if (isPropertyAccessPosition(child)) return; | ||
| if (isInsideNestedFunction(child, callbackNode)) return; |
There was a problem hiding this comment.
Ignores nested callback captures
Medium Severity
findStaleCapturesInCallback skips every identifier inside a nested function within the useCallback body. Reactive values read only in an inner arrow or function are not counted, so empty-deps callbacks that still close over stale props or state can pass the rule.
Reviewed by Cursor Bugbot for commit fef46f8. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -0,0 +1,347 @@ | |||
| import { SETTER_PATTERN, HOOKS_WITH_DEPS } from "../../constants/react.js"; | |||
There was a problem hiding this comment.
🔴 Unused import HOOKS_WITH_DEPS violates AGENTS.md "Remove unused code" rule
HOOKS_WITH_DEPS is imported from ../../constants/react.js on line 1 but is never referenced anywhere in the file. AGENTS.md mandates: "MUST: Remove unused code and don't repeat yourself."
| import { SETTER_PATTERN, HOOKS_WITH_DEPS } from "../../constants/react.js"; | |
| import { SETTER_PATTERN } from "../../constants/react.js"; | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { createComponentPropStackTracker } from "../../utils/create-component-prop-stack-tracker.js"; | ||
| import { collectPatternNames } from "../../utils/collect-pattern-names.js"; | ||
| import { defineRule } from "../../utils/define-rule.js"; | ||
| import { isHookCall } from "../../utils/is-hook-call.js"; |
There was a problem hiding this comment.
🔴 Unused import isHookCall violates AGENTS.md "Remove unused code" rule
isHookCall is imported from ../../utils/is-hook-call.js on line 5 but is never referenced anywhere in the file. AGENTS.md mandates: "MUST: Remove unused code and don't repeat yourself."
| import { isHookCall } from "../../utils/is-hook-call.js"; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (stableNames.has(identifierName)) return; | ||
| if (SETTER_PATTERN.test(identifierName)) return; | ||
| if (isPropertyAccessPosition(child)) return; | ||
| if (isInsideNestedFunction(child, callbackNode)) return; |
There was a problem hiding this comment.
🟡 isInsideNestedFunction blanket-skips reactive values inside nested callbacks, causing false negatives
findStaleCapturesInCallback at line 172 unconditionally skips any identifier inside a nested function (e.g. an arrow function passed to .map(), .forEach(), setTimeout, .then(), etc.). However, reactive values used inside these nested functions ARE still captured by the outer useCallback closure and WILL be stale. For example, useCallback(() => { items.forEach(() => { onSearch(query); }); }, []) — both onSearch and query are stale, but the rule silently ignores them because they're inside the forEach arrow.
Root cause in collectLocalBindings + isInsideNestedFunction mismatch
collectLocalBindings correctly prunes nested functions (return false at line 103) so it does NOT collect parameters of inner callbacks. But findStaleCapturesInCallback's walkAst DOES descend into nested functions and then uses the overly-broad isInsideNestedFunction to skip ALL identifiers it finds there — even those that ARE captured from the outer scope and aren't shadowed by any inner binding. The fix should check whether the identifier is actually shadowed by a parameter or local binding of the intervening function(s), rather than blanket-skipping the entire nested scope.
Prompt for agents
The isInsideNestedFunction check at line 172 in findStaleCapturesInCallback blanket-skips ALL identifiers found inside any nested function within the callback body. This causes false negatives for the common pattern of using reactive values inside .map(), .forEach(), setTimeout, Promise .then() callbacks, etc.
The root cause is a mismatch: collectLocalBindings (line 89-115) correctly stops at nested function boundaries (it returns false for child !== callbackNode at line 103), so it never collects parameters from inner arrow functions. Then findStaleCapturesInCallback's walkAst DOES walk into those nested functions, but isInsideNestedFunction skips all identifiers it finds there — even reactive ones that are genuinely captured from the outer scope.
To fix this, instead of blanket-skipping all identifiers inside nested functions, the code should check whether a given identifier is actually shadowed by a parameter or local binding in any intermediate function scope between the identifier and the callbackNode. One approach:
1. Remove or replace the isInsideNestedFunction check at line 172.
2. Modify collectLocalBindings to also collect parameter names from nested functions (but scoped per-function, not flattened into the same set).
3. Or: at the point of checking each identifier, walk up to the callbackNode collecting parameter/local names from any intermediate function scopes, and skip only if the identifier name is shadowed by one of those intermediate bindings.
This ensures reactive values like props and state used inside .forEach() or setTimeout callbacks are still correctly flagged as stale captures.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Adds a new
no-stale-closurerule (instate-and-effects/) that detects callbacks stabilized viauseCallback(fn, [])oruseRef(fn)that close over reactive values (props, useState, useContext, useReducer state) — meaning those values go stale after the first render.Inspired by React compiler's HIR reactive identifier tracking (
CollectReactiveIdentifiers/PruneNonReactiveDependencies), the rule classifies each component-scope identifier as either reactive (changes between renders) or stable (identity-safe across renders), then checks whether any stabilized callback captures reactive names without being updated.Detection targets:
useCallback(() => { ...reads reactive... }, [])— empty dep array with reactive capturesuseRef(() => { ...reads reactive... })whereref.currentis never reassignedRecommends:
useEffectEvent(React 19+) or auseNonReactiveCallbackhelper that stores the latest callback in a ref viauseInsertionEffect.False-positive avoidance:
useRef/useEffectEvent/useCallback/useMemoreturn bindings are stableset*) are excludedobj.prop) are excludeduseRef(fn)is only flagged whenref.currentis never reassigned in the component bodyIncludes 32 tests covering fail cases, pass cases, and real-world patterns (debounced search, analytics tracking, interval callbacks, zustand selectors, etc.).
Review & Testing Checklist for Human
npx react-doctor@lateston a project after building with this branchuseMemo(() => fn, [])returning a function should also be detected (not currently covered — onlyuseCallbackanduseRef)requires: ["react:19"]likeprefer-use-effect-event, or remain global (current behavior) sinceuseNonReactiveCallbackworks on any React versionNotes
test-noiseso it auto-skips test/story filespnpm gen) —rule-registry.tsupdated automaticallyinstall-agent-hooks.test.ts) is a Node.js version mismatch issue, unrelated to this changeLink to Devin session: https://app.devin.ai/sessions/a9cc7ee589bb4f2287466fe98aa0a660
Note
Medium Risk
New global warn rule may surface many existing patterns; false-positive handling is heuristic (e.g. only empty deps, ref reassignment checks) and needs validation on real codebases.
Overview
Adds a new State & Effects lint rule
no-stale-closurethat warns when component callbacks are frozen with empty deps but still read values that change each render (props,useState/useReducerstate,useContext).It flags
useCallback(fn, [])whenfncloses over reactive bindings, anduseRef(fn)when the ref’s function captures reactive values andref.currentis never reassigned in the component. Diagnostics name the stale identifiers and suggestuseEffectEventor a ref-baseduseNonReactiveCallbackpattern.The rule is registered globally as
react-doctor/no-stale-closure(warn,test-noise), with a large test suite covering pass/fail cases and common real-world patterns (debounced search, interval refs, ref-synced handlers).Reviewed by Cursor Bugbot for commit fef46f8. Bugbot is set up for automated code reviews on this repo. Configure here.