refactor(react-doctor): consolidate state-and-effects rule plumbing#167
Merged
refactor(react-doctor): consolidate state-and-effects rule plumbing#167
Conversation
Cleanup pass over the recently-merged state-and-effects rules from #153–#157, #162 surfacing six audit findings. 1. **De-duplicate constant sets.** Four overlapping allowlists (`SUBSCRIPTION_METHOD_NAMES` ⊂ `EXTERNAL_SYNC_MEMBER_METHOD_NAMES`, `FETCH_MEMBER_OBJECTS` ⊂ `EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS`, `FETCH_CALLEE_NAMES` ⊂ `EXTERNAL_SYNC_DIRECT_CALLEE_NAMES` & `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES`, and the timer/scheduler list shared between `prefer-use-effect-event` and `no-effect-chain`) now spread from a single base set so adding a new HTTP client / subscribe verb in one place propagates everywhere. 2. **Unify `getRootIdentifierName`.** `server.ts` had a private copy that walked through CallExpression chains; the shared helper in `helpers.ts` did not. Added an opt-in `followCallChains` option and deleted the duplicate. 3. **Remove dead branch in `isExternalSyncEffect`.** A redundant `else if (effectCallback.body?.type !== "BlockStatement")` whose body was a comment-only no-op. 4. **Migrate `noDerivedUseState` to the shared root walker.** Drops the inline MemberExpression chain walk and uses `getRootIdentifierName` for consistency. 5. **Drop `noEffectEventHandler`'s deference to `noEventTriggerState`.** The deference predicate only checked two of the five preconditions `noEventTriggerState` actually requires (single dep, handler-only writes, not render-reachable were not verified), so when those failed the narrower rule didn't fire AND this rule deferred — a silent drop. Both rules now fire independently; the two messages frame the same code differently and a duplicate diagnostic is strictly better than a missing one. Also removes ~60 lines of useState-name-tracking machinery that only existed to support the deference. Existing test was updated and a new regression pinning the double-warn was added. 6. **Receiver-gate `push` / `replace`.** Both names live in `EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS` but also denote `Array.prototype.push` and `String.prototype.replace`. Moved them to a separate `EVENT_TRIGGERED_NAVIGATION_METHOD_NAMES` set that's only counted when the receiver root is in `NAVIGATION_RECEIVER_NAMES` (`router`, `navigation`, `navigator`, `history`, `location`). Two regressions added — `arr.push(x)` does not fire, `router.push(path)` still does. All checks green: 617/617 tests, typecheck, lint, format. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
4 tasks
aidenybai
added a commit
that referenced
this pull request
May 8, 2026
…tives (#172) Audit pass on `main` after the consolidation/AGENTS.md cleanups (#167, #169). Targets eight specific FP/FN patterns surfaced by re-reading the merged code end-to-end and checking each rule against its own intent docstring. High - H1 unify release detection between `effectHasCleanupRelease` and `cleanupReleasesSubscription`: both now share `isCleanupReturn` / `isReleaseLikeCall` / `containsReleaseLikeCall`. Previously `prefer-use-sync-external-store` and `prefer-use-effect-event` could disagree on whether a cleanup with a generic teardown verb (`cleanup()`, `dispose()`) counted as a release, producing inconsistent diagnostics. - H2 require function-shaped return for `isExternalSyncEffect`: any `return <expr>` previously qualified the effect as "external sync" and silently disabled chain detection, so `return null` / `return 42` / `return condition && cleanup` would mask real chains. We now only treat function-shaped returns (Arrow, FnExpr, Call, Identifier) as cleanup. - H3 directional version gating: `prefer-use-effect-event` is a "prefer-newer-api" rule and was firing on projects where the React major couldn't be detected. Gating now records whether each rule is "prefer-newer-api" (skip when version is unknown) or "deprecation-warning" (keep firing when version is unknown). Medium - M1 receiver-gate ambiguous direct callees: `track`, `logEvent`, `del` removed from `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES` — they were too generic as bare identifiers and produced FPs on user-defined helpers. Receiver-gated member calls (`analytics.track(...)`) still fire. - M2 extend `findSubHandlerForEnclosingFunction` to FunctionDeclaration and AssignmentExpression shapes — `function handler() {}` and `let h; h = (e) => {}` are now recognized alongside `const h = ...`. - M3 deep-walk for `noPropCallbackInEffect`: the rule previously only scanned top-level effect statements and missed the very common `if (didChange) onChange(state)` shape. Walk now descends through control-flow blocks but stops at function boundaries (so deferred sub-handlers like `setTimeout(() => onChange(state))` stay the domain of `prefer-use-effect-event`). - M4 `collectWrittenStateNamesInEffect` no longer counts setter calls inside nested functions for chain detection — deferred writes (`setTimeout(() => setX(...))`) are not synchronous chain triggers. Low - L1 `noMirrorPropEffect` now accepts multi-element deps as long as the mirrored prop's root is in the deps array. The prior "exactly one dep" requirement missed legitimate mirror shapes with an unrelated extra dep. - L2 (paired with H1/H2) `effectHasCleanupRelease` "return Identifier" now narrows to known bound release names so a stray non-function identifier return doesn't silently look like a release. - L3 extend `prop-stack-barrier.test.ts` to cover all four `createComponentPropStackTracker` consumers (`no-derived-useState`, `no-prop-callback-in-effect`, `no-mirror-prop-effect`, `prefer-use-effect-event`) so the empty-frame barrier semantic is regression-tested for every rule that depends on it. - L4 extract a sibling `createComponentBindingStackTracker` and migrate `noEffectEventInDeps` onto it — the third inlined push/pop scaffold was effectively a copy-paste of the prop-stack one specialised to binding sets. Tests - 634 passing (up from 619). New regressions cover each behavior change above plus FP guards (e.g. mirror shape with prop root NOT in deps must not fire, sub-handler reads must not fire `no-prop-callback-in-effect`). - `tests/run-oxlint.test.ts` now passes `reactMajorVersion: 19` for the basic / Next / TanStack Start fixtures so they exercise the prefer-newer-api rules under a known React major. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleanup pass over the recently-merged state-and-effects rules from #153–#157, #162. Surfaced six audit findings; this PR fixes all of them.
getRootIdentifierNameduplicated inhelpers.tsandserver.tswith different semanticsfollowCallChainsto the shared helper, delete the duplicateelse ifbranch inisExternalSyncEffectnoDerivedUseStatereimplements MemberExpression chain walkgetRootIdentifierNamenoEffectEventHandlerdeference predicate could silently drop diagnostics whennoEventTriggerState's narrower preconditions failedEVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODSincludespush/replace, conflicting withArray.prototype.pushandString.prototype.replaceNAVIGATION_*set that's only counted when the receiver root looks router-shapedTest plan
pnpm format— cleanpnpm typecheck— cleanpnpm lint— 0 warnings, 0 errorspnpm test— 617/617 passing (615 prior + 2 new regressions:router.pushstill fires,arr.pushdoes not)Made with Cursor
Note
Medium Risk
Touches multiple
react-doctorlint rule detectors and their shared allowlists; behavior changes can affect which diagnostics users see (including intentional double-warns and new navigation heuristics). Risk is contained to static analysis but may introduce false positives/negatives if the new name gating is off.Overview
Consolidates and de-duplicates shared allowlists used across state/effect rules by factoring timers/schedulers, fetch/HTTP client shorthands, and subscription method names into base sets that other detectors spread from, reducing drift between rules.
Unifies root-identifier resolution by enhancing
getRootIdentifierNamewith an opt-infollowCallChainsmode and deleting a duplicate implementation inserver.ts; rules likeserverDedupProps,noDerivedUseState, and external-sync detection now reuse the shared helper.Adjusts rule behavior to avoid missed or noisy diagnostics:
no-effect-event-handlerno longer defers tono-event-trigger-state(allowing intentional double warnings rather than silent drops), andno-event-trigger-statenow treatspush/replaceas side effects only for router-shaped receivers (avoids false positives onArray.prototype.push/String.prototype.replace). Adds regression tests for these cases and removes dead logic in external-sync effect detection.Reviewed by Cursor Bugbot for commit c7b4c18. Bugbot is set up for automated code reviews on this repo. Configure here.